-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat(i): Handle collection commits over P2P #3247
feat(i): Handle collection commits over P2P #3247
Conversation
8a603cd
to
473b681
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3247 +/- ##
===========================================
+ Coverage 77.54% 77.58% +0.04%
===========================================
Files 382 382
Lines 35263 35301 +38
===========================================
+ Hits 27344 27386 +42
+ Misses 6291 6290 -1
+ Partials 1628 1625 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 22 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! The implementation is simple considering the complexity of the feature.
internal/db/merge.go
Outdated
mp, err := db.newMergeProcessor(txn, col, dsKey) | ||
if err != nil { | ||
return err | ||
var mt mergeTarget |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: saves a few lines
var key keys.HeadstoreKey
if dagMerge.DocID != "" {
key = keys.HeadstoreDocKey{...}
} else {
key = keys.NewHeadstoreColKey(...)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol, my bad - thanks for the suggestion Keenan 😁
- Simplify executeMerge key block
Results: map[string]any{ | ||
"commits": []map[string]any{ | ||
{ | ||
"cid": testUtils.NewUniqueCid("collection"), | ||
"cid": testUtils.NewUniqueCid(0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I was playing around here using ints as cid ids and forgot it was an existing test - I still prefer strings, but let me know your thoughts :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the input param needed or could the function generate a random ID in the NewUniqueCid
func?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The input param is purely descriptive, it has no relation to the value of the cid that it represents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although, if I misunderstood you - yes there needs to be a value as otherwise there is no way to describe which values should/shouldn't be the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ask because if generated internally the pattern for reuse could be:
cidA := testUtils.NewUniqueCid()
cidB := testUtils.NewUniqueCid()
Just a thought I had, no need to change the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I guess we could just make it a pointer and check for equality of the reference - would be a better non-descriptive version than using int
, but a fair bit more wordy as you'd need to declare the variables if used in multiple places. (I still prefer strings atm though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The strings definitely make this more comprehensible. I much prefer how it was before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change this test back to strings in a sec, I can revert if anyone comes back very soon before merge in favour of the ints @islamaliev
- Use strings in test
55177fc
to
a417d68
Compare
a417d68
to
d1906b2
Compare
internal/db/messages.go
Outdated
@@ -22,7 +22,9 @@ import ( | |||
) | |||
|
|||
func (db *db) handleMessages(ctx context.Context, sub *event.Subscription) { | |||
queue := newMergeQueue() | |||
docIdQueue := newMergeQueue() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: docIDQueue
would be more idomatic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 years later and I'm still messing that up 😁
- docIDQueue
tests/integration/acp.go
Outdated
@@ -215,7 +215,12 @@ func addDocActorRelationshipACP( | |||
} | |||
|
|||
if action.ExpectedError == "" && !action.ExpectedExistence { | |||
waitForUpdateEvents(s, actionNodeID, map[string]struct{}{docID: {}}) | |||
expect := make([]map[string]struct{}, action.CollectionID+1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: You prefer creating a CollectionID+1
lenght array that will have just one item instead of passing the collection ID to the waitForUpdateEvents
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started working from the waitForUpdateEvents
(and late, as a PR fixup). waitForUpdateEvents
was multi-collection friendly and so I preserved that without thinking too hard about it, although I think you are right and that all the callers are dealing with a single collection.
Your suggestion makes a lot of sense :) Although I think I might be able to pass a collection instead of just the index.
- Simplify
waitForUpdateEvents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT. Good job Andy. Just. one suggestion to improve readability of the expected result.
schemaVersionKey := keys.CollectionSchemaVersionKey{ | ||
SchemaVersionID: mp.col.Schema().VersionID, | ||
CollectionID: mp.col.ID(), | ||
} | ||
|
||
if field == "" { | ||
mcrdt = merklecrdt.NewMerkleCompositeDAG( | ||
switch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: Nice use of the block's IsFoo
methods.
"cid": testUtils.NewUniqueCid("collection, update3"), | ||
"links": []map[string]any{ | ||
{ | ||
"cid": testUtils.NewUniqueCid("collection, update2"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: If you keep the node in the string (collection, node0 update2
) it would help to know where this update comes from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh true, I dropped that forgetting that the update was actually applied to a single node (at first). Will do, thanks Fred.
- note update node in cid description
Relevant issue(s)
Resolves #3212
Description
Handles the syncing of collection commits over P2P.
Note: The behaviour inDiscussed, and test updated.TestQueryCommitsBranchables_HandlesConcurrentUpdatesAcrossPeerConnection
is currently under discussion and may be changed or linked to #3245 and #3246 before this PR is merged.Todo