-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
chore(storage): round up Chunksize to nearest multiple of 256kib [GRPC] #7799
Conversation
|
||
// Round up chunksize to nearest 256KiB | ||
if size%googleapi.MinUploadChunkSize != 0 { | ||
size += googleapi.MinUploadChunkSize - (size % googleapi.MinUploadChunkSize) |
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 would probably round by doing size = MinUploadChunkSize * (1 + size/MinUploadChunkSize)
, but either way works I guess
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.
And, the if statement is superfluous.
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.
The if statement is needed when a user provides an exact multiple of 256kib. Without the if statement, the chunksize would be chunksize+256kib
instead of just chunksize
.
I am inclined to keep the code as-is since it's copied directly from here; might as well keep it consistent.
chunksize int | ||
wantBytesPerCall int64 | ||
}{ | ||
{ |
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.
Let's also check that progressfunc is called the expected number of times for each case.
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.
Good call, adding that uncovered a discrepancy we have between HTTP and GRPC - the progress function is only called in HTTP if the upload is resumable (so it is not called at all for a one-shot), in GRPC the function is called at least once even if the upload is not resumable.
The docs for the progress func do specify it gets called only if the upload requires more than one call, so I assume we want to modify gRPC to comply with this?
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.
Agreed, that sounds right.
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.
Opened #7839 to keep track
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.
One other test thing, otherwise lgtm
storage/integration_test.go
Outdated
}, | ||
{ | ||
desc: "small chunksize rounds up to 256kib", | ||
chunksize: 1, | ||
wantBytesPerCall: 256 << 10, | ||
wantCallbacks: int(math.Ceil(float64(objSize) / (256 << 10))), |
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 is confusing and opaque, just put the literal number of expected calls here I think.
Fixes #7754
GRPC skipped due to #7798 affecting the last test
chunksize 0 uploads everything
Also cleans up a typo I noticed in ProgressFunc docs