[7/N] Chunk encoding optimization: Node support for mixed encoding #654
Conversation
| @@ -307,6 +307,26 @@ func (s *Server) RetrieveChunks(ctx context.Context, in *pb.RetrieveChunksReques | |||
| s.node.Metrics.RecordRPCRequest("RetrieveChunks", "failure", time.Since(start)) | |||
| return nil, fmt.Errorf("could not find chunks for batchHeaderHash %v, blob index: %v, quorumID: %v", batchHeaderHash, in.GetBlobIndex(), in.GetQuorumId()) | |||
| } | |||
| if !s.config.EnableGnarkBundleEncoding && format == pb.ChunkEncoding_GNARK { | |||
There was a problem hiding this comment.
there would be a case when gnark blob is enabled but, the client forgot to set the flag. why not make two interfaces altogether, and remove the flag?
If this happens, the system might be able to store data, but not be able to retrieve. It would be a safety violation
There was a problem hiding this comment.
Having a separate interface (from RetrieveChunks) will still have the issue of "old-new" communication: if a new client talks to an old server (which doesn't have the new API), it will fail.
| QuorumId: 1, | ||
| }) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, pb.ChunkEncoding_UNKNOWN, retrievalReply.Encoding) |
There was a problem hiding this comment.
nice, so you handled the case when bundle is empty when operator is not in that quorum. But you also return the unknown when blob cannot be retrieved, but since you return the error there,so it would fine I think
There was a problem hiding this comment.
It should be fine, empty bundle has no header to tell what's the encoding format anyway.
| @@ -388,6 +498,7 @@ func TestRetrieveChunks(t *testing.T) { | |||
| QuorumId: 0, | |||
| }) | |||
| assert.NoError(t, err) | |||
| assert.Equal(t, pb.ChunkEncoding_GOB, retrievalReply.Encoding) | |||
There was a problem hiding this comment.
why is it gob, since the storeChunkfunction is true?
There was a problem hiding this comment.
I think in the node/store.go func DecodeChunks, you regard format 0 as ChunkEncoding_GOB. It is confusing since it is sometimes 0 sometimes 2.
There was a problem hiding this comment.
but anyway, shouldn't the format be 1, since it is requested?
There was a problem hiding this comment.
By default, EnableGnarkBundleEncoding flag is false, so the node will continue to return GOB, even though the underlying data stored in DB is in GNARK.
The ID for protobuf and the enum in golang are different for a reason, added the documentation: 7978389
| } | ||
| bundles[uint8(quorumID)] = bundleMsg | ||
| } else { | ||
| bundles[uint8(quorumID)] = make([]*encoding.Frame, 0) |
There was a problem hiding this comment.
good catch. I think it is handling the case, when there is quorum which the operator does not belong to. But I thought you already catch it here, https://github.com/Layr-Labs/eigenda/blob/master/disperser/batcher/grpc/dispatcher.go#L376
There was a problem hiding this comment.
I guess the question is if the grpc would return a nil, or empty array.
There was a problem hiding this comment.
If the operator is not in this quorum, then the bundle is an empty []byte (not nil), so len(bundle.GetBundle()) is 0
Why are these changes needed?
To make the Node able to serve both Gob and Gnark chunks, and return only Gob chunks during migration -- guarded by a flag.
This also added more test coverage for the mixed encoding which can be the case during migration.
Checks