Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Integrate CARv2 blockstore in the retrieval market #560
Integrate CARv2 blockstore in the retrieval market #560
Changes from 8 commits
dd70879
7435c9f
932f6c0
b47f9e6
fb18a6b
07597f8
92e827d
9c440e2
9e66010
79305a7
4907775
7063c15
5e21789
7d01608
7c81e47
296d14c
9a2f61b
6c5210e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
No, we can't rely on the DAGStore to do this.
The idea is for the DAGStore to use it for traceability etc. in the future and for the dagstore to not leave a go-routine hanging if you've cancelled your context and moved on.
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.
Not at the moment, but it's on the roadmap: filecoin-project/dagstore#39. If we need it now, then we should prioritise it.
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.
@raulk It's a nice to have for sure because then we don't need to put that code here. Let's prioritize it.
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.
Use filecoin-project/dagstore#33 when it's ready here. If user has deleted an Index file from the filesystem for example, the acquire will fail. No reason we can't re-index and access the shard by using recovery here.
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.
Use filecoin-project/dagstore#33 when it's ready here. If user has deleted an Index file from the filesystem for example, the acquire will fail. No reason we can't re-index and access the shard by using recovery here.
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.
So, the
CarReadOnlyStoreTracker
will take care of closing this accessor as a part of theCleanBlockstore
call, right ?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.
Right 👍
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.
Nope, we should do it ourselves.
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.
Please don't work around the inexistence of a feature, and instead build it in the right place: filecoin-project/dagstore#39.
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.
Hook point for the shard recovery feature when it's implemented this week.
filecoin-project/dagstore#33