Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to db2 #96

Merged
merged 64 commits into from
Oct 31, 2023
Merged

Upgrade to db2 #96

merged 64 commits into from
Oct 31, 2023

Conversation

Powersource
Copy link
Contributor

@Powersource Powersource commented Oct 13, 2023

For #90

  • remove all direct usage of keystore
  • uninstall keyring
  • remove old deps

@socket-security
Copy link

socket-security bot commented Oct 13, 2023

New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
ssb-box2 7.2.0 None +2 160 kB
pull-many 1.0.9 None +0 11.6 kB dominictarr
ssb-crut 4.6.3...5.1.0 None +0/-0 138 kB mixmix

🚮 Removed packages: [email protected], [email protected], [email protected]

Copy link
Member

@mixmix mixmix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far! I'm impressed by how much progress you've made so far

README.md Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
listen.js Show resolved Hide resolved
method/group.js Show resolved Hide resolved
method/link.js Outdated Show resolved Hide resolved
rebuild-manager.js Outdated Show resolved Hide resolved
test/helpers/test-bot.js Show resolved Hide resolved
test/helpers/test-bot.js Show resolved Hide resolved
@Powersource Powersource mentioned this pull request Oct 17, 2023
2 tasks
@Powersource

This comment was marked as outdated.

package.json Outdated Show resolved Hide resolved
@@ -42,7 +42,8 @@ function fromMsgKey (msg, msgKey) {
}

function fromGroupKey (msg, groupKey) {
const { author, previous, content } = msg.value
const { author, previous } = msg.value
const content = (msg.meta && msg.meta.originalContent) || msg.value.content
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const content = (msg.meta && msg.meta.originalContent) || msg.value.content
const content = msg?.meta?.originalContent || msg.value.content

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've avoided ?. to make it easier to potentially support node 12 again in the future, but maybe we've dropped the hopes for that?

listen.js Outdated Show resolved Hide resolved
listen.js Outdated Show resolved Hide resolved
listen.js Outdated Show resolved Hide resolved
listen.js Show resolved Hide resolved
method/group.js Outdated Show resolved Hide resolved
method/group.js Outdated Show resolved Hide resolved
keystore.poBox.add(poBoxId, { key: secret }, (err) => {
if (err) return cb(err)
ssb.box2.addPoBox(poBoxId, { key: secret }, (err) => {
if (err) return cb(Error("Couldn't add pbox to box2 when adding pobox", { cause: err }))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (err) return cb(Error("Couldn't add pbox to box2 when adding pobox", { cause: err }))
if (err) return cb(Error("ssb.tribes.addPoBox failed in ssb-box2", { cause: err }))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or something

method/link.js Outdated
Comment on lines 83 to 84
parent && equal(seekParent, parent, { indexType: 'parent', prefix: true }),
child && equal(seekChild, child, { indexType: 'child', prefix: true }),
Copy link
Member

@mixmix mixmix Oct 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
parent && equal(seekParent, parent, { indexType: 'parent', prefix: true }),
child && equal(seekChild, child, { indexType: 'child', prefix: true }),
parent && equal(seekParent, parent, { indexType: 'linkParent', prefix: true }),
child && equal(seekChild, child, { indexType: 'linkChild', prefix: true }),

going for more specific index names by default - to avoid collisions with any other modules written
BTW I am new the the prefix: true thing. It seems like this is the right choice here, did you ask any of the butt-db elders about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will change the index names.

didn't ask about prefix, i was hoping you were familiar 😁 i got the impression from the db2 or jitdb readme that prefix was good here but i wasn't 100%. what do you say @staltz ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed index names

parent && equal(seekParent, parent, { indexType: 'parent', prefix: true }),
child && equal(seekChild, child, { indexType: 'child', prefix: true }),
equal(seekTanglesLinkRoot, null, { indexType: 'tanglesLinkRoot', prefix: true }),
equal(seekTanglesLinkPrevious, null, { indexType: 'tanglesLinkPrevious', prefix: true })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌶️

I think this index might be redundant, as it's very unlikely to have root: null and previous: [a, b]
I think we could do a pull.filter check instead of building an index for all the previous fields....? butt I'm not an index expert?

dbType('link/feed-group'),
equal(seekParent, feedId, { indexType: 'parent', prefix: true }),
equal(seekTanglesLinkRoot, null, { indexType: 'tanglesLinkRoot', prefix: true }),
equal(seekTanglesLinkPrevious, null, { indexType: 'tanglesLinkPrevious', prefix: true })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dito

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
t.end()
})
})
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any cache on the group tangle still?
it's kinda important to stop clobbering the DB I think - we have some bulk record creation from import supported, can make 1000 messages easily in one go. Cannot re-build tangle per message!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we removed it also in tribes2, i think we said db2 is so good at caching anyway

Comment on lines +35 to +37
await p(setTimeout)(500)

t.true(me.status().sync.sync, 'all indexes updated')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new pattern I've been using for this sorta wait :

Suggested change
await p(setTimeout)(500)
t.true(me.status().sync.sync, 'all indexes updated')
while (!me.status().sync.sync) await p(setTimeout)(100)
t.pass('all indexes updated')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we're removing this in #105 anyway since it's db1 stuff

Copy link
Member

@mixmix mixmix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of minor things, but this was a very exciting and satisfying PR to read.
So awesome seeing this all tuned up and stream lined.
Fantastic work

listen.js Outdated
dbLive({ old: true }),
toPullStream()
),
ssb.db.reindexed(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing the filter from reindexed here seems to let weird stuff through

addNewAuthorListener
    ✔ admin = returns expected newAuthors
    ✔ admin = returns expected groupId
    ssb-tribes/admin ⇒ ssb-tribes/newPerson
    ✔ admin sees newMember being added
    ✔ admin = returns expected groupId
      1 [?]
      2 [?]
      3 group/add-member  ─ ssb-tribes/newPerson
    ✔ newPerson returns expected newAuthors
/home/runner/work/ssb-tribes/ssb-tribes/index.js:150
    const { poBoxId, key: poBoxKey } = m.value.content.keys.set
                                                            ^

TypeError: Cannot read properties of undefined (reading 'set')
    at /home/runner/work/ssb-tribes/ssb-tribes/index.js:150:61
    at /home/runner/work/ssb-tribes/ssb-tribes/node_modules/pull-stream/sinks/drain.js:32:37
    at /home/runner/work/ssb-tribes/ssb-tribes/node_modules/pull-stream/throughs/filter.js:17:11
    at /home/runner/work/ssb-tribes/ssb-tribes/node_modules/pull-stream/throughs/filter.js:17:11
    at /home/runner/work/ssb-tribes/ssb-tribes/node_modules/pull-stream/throughs/filter.js:17:11
    at check (/home/runner/work/ssb-tribes/ssb-tribes/node_modules/pull-many/index.js:72:16)
    at next (/home/runner/work/ssb-tribes/ssb-tribes/node_modules/pull-many/index.js:104:21)
    at callback (/home/runner/work/ssb-tribes/ssb-tribes/node_modules/pull-pushable/index.js:84:5)
    at Function.push (/home/runner/work/ssb-tribes/ssb-tribes/node_modules/pull-pushable/index.js:44:7)
    at notify (/home/runner/work/ssb-tribes/ssb-tribes/node_modules/pull-notify/index.js:12:20)
    ✔ newPerson, returns expected groupId

  test count(6) != plan(null)

  Failed tests: There was 1 failure

    ✖ undefined) undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-added the removed filter 2f01a5f

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made issue for followup #107

@Powersource
Copy link
Contributor Author

merging since approved and passing

@Powersource Powersource merged commit f324d2a into master Oct 31, 2023
7 checks passed
@Powersource Powersource deleted the db2 branch October 31, 2023 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants