[cas] Implement batch upload #307
Conversation
| bytesTransferred += r.Digest.SizeBytes | ||
| digestsTransferred++ | ||
| } | ||
| atomic.AddInt64(&u.stats.Batched.Bytes, bytesTransferred) |
There was a problem hiding this comment.
If we use atomic package to update this value, better to make this private and provide accessor using atomic.Load*?
Or use package like https://pkg.go.dev/go.uber.org/atomic ?
There was a problem hiding this comment.
This field is private until it is done updating. We don't update it after we publish it.
There was a problem hiding this comment.
I think that won't be matter in practice, but golang/go#5045 (comment) says
you shouldn't mix atomic and non-atomic accesses for a given memory word.
There was a problem hiding this comment.
yes, it says that and we indeed must follow those rules for concurrent reads and writes. But this is not the case here.
To be be clear: uploader is a private struct. Other packages do not have access to it. Once we return the stat, we no longer write.
There was a problem hiding this comment.
I think copying value by return is not guaranteed to be valid operation too if atomic is used there?
There was a problem hiding this comment.
I don't think so. Do you have links to some documentation to backup this up?
There was a problem hiding this comment.
Seems like this is the only outstanding comment and I am pretty sure this code is correct. I am happy to fix it in another CL if it is not.
go/pkg/cas/client.go
Outdated
|
|
||
| // BatchUpdateBlobsBatchSize is the maximum number of blobs to upload in a | ||
| // single BatchUpdateBlobs RPC. | ||
| BatchUpdateBlobsBatchSize int |
There was a problem hiding this comment.
BatchBlobsMaxBatchSize? I think the RE specifies the same max for both upload and download.
There was a problem hiding this comment.
Done, but should they have the same defaults though? The default value for this field was designed for writes. Now the same value is used for reads. This seems a bit arbitrary?
Partially implement scheduleUpload - for small blobs. Use bundler.Bundler to bundle blobs together.
Partially implement scheduleUpload - for small blobs.
Use bundler.Bundler to bundle blobs together.