[v2] Encoding manager #846
Conversation
ian-shim
force-pushed
the
v2-encoding-manager
branch
from
4c9c822 to
02cc5ff
Compare
|
To be merged on top of #844 |
| // NumEncodingRetries defines how many times the encoding will be retried | ||
| NumEncodingRetries int | ||
| // NumRelayAssignment defines how many relays will be assigned to a blob | ||
| NumRelayAssignment uint16 |
There was a problem hiding this comment.
nit: seems like this should be plural
| relayKeys := availableRelays | ||
| // shuffle relay keys | ||
| for i := len(relayKeys) - 1; i > 0; i-- { | ||
| j := rand.Intn(i + 1) |
There was a problem hiding this comment.
should this be cryptographically secure randomness? If so crypto/rand should be used.
There was a problem hiding this comment.
I don't think so. It just has to be random enough. Performance is probably more important than security
| e.pool.Submit(func() { | ||
| for i := 0; i < e.NumEncodingRetries+1; i++ { | ||
| encodingCtx, cancel := context.WithTimeout(ctx, e.EncodingRequestTimeout) | ||
| fragmentInfo, err := e.encodeBlob(encodingCtx, blobKey, blob) |
There was a problem hiding this comment.
Is this for adding metadata for how to retrieve from the S3 bucket?
There was a problem hiding this comment.
Yeah current thinking is that encoder returns the size of the chunks and the number of fragments (files stored), and controller stores them in metadata. Wdyt?
There was a problem hiding this comment.
Yeah I see, it makes sense to me now since we are writing the chunks through the S3 client with the new fragmenting logic.
ian-shim
force-pushed
the
v2-encoding-manager
branch
from
02cc5ff to
1d25e75
Compare
|
|
||
| // Encode the blobs | ||
| e.pool.Submit(func() { | ||
| for i := 0; i < e.NumEncodingRetries+1; i++ { |
There was a problem hiding this comment.
I don't really love how this all uses one retry loop. If we are just failing to update dynamo on the last step, do we really need to reencode the blob?
There was a problem hiding this comment.
The plan was to keep encoder request idempotent (i.e. encoder checks if the blob has been encoded and returns immediately in that case). Then the extra encoding request is trivial
There was a problem hiding this comment.
In order to keep the encoder request idempotent that means we do something like ListObject on s3 and make sure stuff exists there?
cc: @cody-littley we were discussing this part in the morning.
There was a problem hiding this comment.
Hmm, that could be pretty slow. This seems a bit unideal. @ian-shim , do you have thoughts?
There was a problem hiding this comment.
We don't have to list whole bucket. Since we already know the object names, we can check if those objects exist right?
| } | ||
|
|
||
| storeCtx, cancel = context.WithTimeout(ctx, e.StoreTimeout) | ||
| err = e.blobMetadataStore.MarkBlobEncoded(storeCtx, blobKey, fragmentInfo) |
There was a problem hiding this comment.
Why are we updating the blob record with the fragmentInfo instead of just pushing it next to the BlobCert?
There was a problem hiding this comment.
It would be nicer to put it in BlobCert. I'm not sure if it belongs there though. Is it something that should go onchain?
There was a problem hiding this comment.
Couldn't we just store a wrapper at this stage which contains both the BlobCert and this additional metadata? I agree it should not be in the actual canonical BlobCert structure.
There was a problem hiding this comment.
My worry is that now there are two different data structures representing BlobCert (one used in disperser & relay, and another one in node). I'd rather keep one consistent struct across. Is updating the blob metadata that bad?
There was a problem hiding this comment.
Actually, ignore that. I ended up marshaling/unmarshaling them separately and putting them in BlobCertificate: e76e3a6
ian-shim
force-pushed
the
v2-encoding-manager
branch
2 times, most recently
from
8825f41 to
59fbc1c
Compare
ian-shim
force-pushed
the
v2-encoding-manager
branch
from
59fbc1c to
f1bd032
Compare
Why are these changes needed?
Checks