[v2] Node DownloadBatch method
#880
Conversation
core/v2/validator.go
Outdated
| type BlobShard struct { | ||
| BlobCertificate | ||
| Chunks map[core.QuorumID][]*encoding.Frame | ||
| *BlobCertificate | ||
| Chunks map[core.QuorumID]core.Bundle |
There was a problem hiding this comment.
@mooselumph can you comment if you like this change?
|
This PR depends on #875 |
ian-shim
force-pushed
the
node-v2-store-chunks
branch
5 times, most recently
from
ab183e3 to
3835f68
Compare
core/serialization.go
Outdated
| // GetBatchHeader constructs a core.BatchHeader from a proto of pb.StoreChunksRequest. | ||
| // Note the StoreChunksRequest is validated as soon as it enters the node gRPC | ||
| // interface, see grpc.Server.validateStoreChunkRequest. | ||
| func BatchHeaderFromProtobuf(in *pb.BatchHeader) (*BatchHeader, error) { |
There was a problem hiding this comment.
nit: BatchHeaderFromProto to be consistent (or BlobHeaderFromProtobuf below if full spell is preferred)
core/v2/types.go
Outdated
| @@ -211,6 +211,77 @@ type Batch struct { | |||
| BlobCertificates []*BlobCertificate | |||
| } | |||
|
|
|||
| func (b *Batch) ToProtobuf() (*commonpb.Batch, error) { | |||
| if b.BatchHeader == nil { | |||
| return nil, fmt.Errorf("batch header is nil") | |||
There was a problem hiding this comment.
nit: here and below, errors.New since there is not formatting param
core/v2/validator.go
Outdated
| BlobCertificate | ||
| Chunks map[core.QuorumID][]*encoding.Frame | ||
| *BlobCertificate | ||
| Bundles map[core.QuorumID]core.Bundle |
node/node_v2.go
Outdated
| Bundles map[core.QuorumID][]byte | ||
| } | ||
|
|
||
| func (n *Node) DownloadBatch(ctx context.Context, batch *corev2.Batch) ([]*corev2.BlobShard, []*RawBundles, error) { |
There was a problem hiding this comment.
It looks odd to supply the batch as input parameter, while the goal is to produce (download) a batch (unless the parameter is also used to bring out the output which isn't the case here)
There was a problem hiding this comment.
Agree with the confusion. I'll rename this method to DownloadBundles. Is that more clear?
| } | ||
| pool.StopWait() | ||
|
|
||
| for i := 0; i < len(requests); i++ { |
There was a problem hiding this comment.
The pool may send less than len(requests) responses to the channel as it exits on error (L103), so this may hang/stuck the server.
There was a problem hiding this comment.
Thanks for catching this. Fixed!
| } | ||
| type relayRequest struct { | ||
| chunkRequests []*clients.ChunkRequestByRange | ||
| metadata []*requestMetadata |
There was a problem hiding this comment.
This looks not metadata, but an integral part of the request itself (without quorum ID, the request is incomplete). That is, a well defined request should have blobkey, chunk index range and quorum ID all together.
There was a problem hiding this comment.
This is metadata for keeping track of the requests, i.e. the actual request to relay doesn't require quorum IDs, just blob key and indices. This metadata is used on the client side (node) to put the responses in the right place.
node/node_v2.go
Outdated
| Bundles map[core.QuorumID][]byte | ||
| } | ||
|
|
||
| func (n *Node) DownloadBatch(ctx context.Context, batch *corev2.Batch) ([]*corev2.BlobShard, []*RawBundles, error) { |
There was a problem hiding this comment.
It seems making robust retry logic for this method could become pretty complex. Are you thinking of doing this as a follow-on/post-MVP?
There was a problem hiding this comment.
I was thinking about not retrying at all (at least for the MVP) and returning error immediately if download fails. What do you think?
There was a problem hiding this comment.
Since this is node software that can't be upgraded as swiftly, I wonder if it may be worth thinking about whether we want to build in some kind of robustness logic from the initial release. 🤔 But definitely as a follow-up after we have the system running (maybe that's your point)
ian-shim
force-pushed
the
node-v2-store-chunks
branch
3 times, most recently
from
4a32367 to
f043837
Compare
ian-shim
force-pushed
the
node-v2-store-chunks
branch
from
f043837 to
1c4d6e7
Compare
Why are these changes needed?
Implements a utility method for downloading a batch from relays.
Checks