-
Notifications
You must be signed in to change notification settings - Fork 51
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
chore: Change from ipld traversal to direct link access #2931
chore: Change from ipld traversal to direct link access #2931
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2931 +/- ##
===========================================
- Coverage 79.38% 79.34% -0.04%
===========================================
Files 326 326
Lines 24769 24778 +9
===========================================
- Hits 19661 19659 -2
- Misses 3696 3704 +8
- Partials 1412 1415 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 15 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.
LGTM :)
6b05fa7
to
4901737
Compare
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.
Looks great! Now it's straightforward to comprehend. I don't understand why we used that convoluted selector/builder/walker approach.
if ctx.Err() != nil { | ||
return | ||
} | ||
nd, err := lsys.Load(linking.LinkContext{Ctx: ctx}, lnk, coreblock.SchemaPrototype) |
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: with this change doesn't it mean we can read directly from datastore.Blockstore
in mergeProcessor.loadComposites
instead of pulling it from block service?
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.
loadComposites
doesn't use a block service. It already reads directly from the blockstore.
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.
alright, I see
asyncErrOnce.Do(func() { setAsyncErr(err) }) | ||
return | ||
} | ||
err = loadBlockLinks(ctx, lsys, linkBlock) |
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.
questions: are we supposed to traverse the whole tree every time an update happens? Shouldn't we stop once we read an existing block?
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 benefit for the moment is that partial syncs can be completed at a later time. If we stop when we have a known block, then completing partial syncs won't be possible until we implement some tracking logic either for the completely synced or partially synced branches. The implementation before this PR did a complete traversal as well.
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.
okay. Thanks for clarification.
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.
LGTM, just a question
4901737
to
36ad742
Compare
Relevant issue(s)
Resolves #2930
Description
This PR removes the use of the ipld traversal function in favor of direct link access. This makes the sync process a bit more efficient and will make it easier to initiate encryption key exchange.
Tasks
How has this been tested?
make test
Specify the platform(s) on which this was tested: