-
Notifications
You must be signed in to change notification settings - Fork 244
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
Refactor piece and segment downloading into subspace-data-retrieval #3062
Conversation
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.
Notes from review with Nazar:
- don't change
subspace-service
- don't refactor, just copy the code (and note where it came from)
3dc1ad0
to
ee6eb5a
Compare
During my manual testing, I found some bugs in piece and segment index calculations, and added some trace logging. We can remove the logging later, once we've finished automated testing. |
tracing::debug!( | ||
%piece_index, | ||
piece_offset, | ||
RawRecord_SIZE = RawRecord::SIZE, | ||
"Invalid piece offset for object: must be less than the size of a raw record", | ||
); |
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'd say we don't need these logs if we already return an error explicitly. It balloons the code side and brings little to no benefit.
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 agree the logs don’t need to be in there long-term, but I’d like to keep them temporarily while I’m testing and debugging this code.
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.
Thanks for the review, just some follow-up questions.
tracing::debug!( | ||
%piece_index, | ||
piece_offset, | ||
RawRecord_SIZE = RawRecord::SIZE, | ||
"Invalid piece offset for object: must be less than the size of a raw record", | ||
); |
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 agree the logs don’t need to be in there long-term, but I’d like to keep them temporarily while I’m testing and debugging this code.
a6f0b39
to
dd0b98f
Compare
// We want exact pieces, so any errors are fatal. | ||
let received_pieces: Vec<Piece> = received_pieces.try_collect().await?; |
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.
This will work for now, but long-term this generic function isn't very useful IMO because you will want to do reconstruction of missing pieces instead of failing altogether here. We already do this in farmer for example.
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.
Thanks, I'll put that on the list.
Purpose
This PR implements piece and segment fetching and reconstruction in the
subspace-data-retrieval
crate, so we can use that code to fetch objects.Change List
This PR refactors piece and segment downloading into the
subspace-data-retrieval
crate.It also:
combines theDsnSyncPieceGetter
andObjectPieceGetter
traitsObjectFetcher
And makes some minor cleanups:
removes some debugging code that would cause a dependency onsubspace-networking
Out of Scope
It doesn't implement piece retries yet, I think that can go in another PR.
Testing
I've manually tested this PR with a proof of concept object fetching RPC on a local node in dev mode. The objects come back correctly and pass a hash check. (I haven't tested anything large or over multiple segments yet.)
Code contributor checklist: