Repository navigation
feat(gax): align resumable upload chunk size to server granularity and surface effective chunk size - #14670
feat(gax): align resumable upload chunk size to server granularity and surface effective chunk size#14670whowes wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the concept of an 'effective chunk size' to the resumable upload process, aligning the configured chunk size with the server's chunk granularity and exposing it via ResumableUploadFuture and ResumableUploadProgress. Feedback on these changes highlights two key concurrency and safety improvements in ResumableUploadFutureImpl.java: first, avoiding a potential NullPointerException by formatting the nullable effectiveChunkSize with %s instead of %d and localizing volatile reads; second, ordering volatile writes so that effectiveChunkSize is written before uploadSessionUrl to ensure consistent visibility across threads.
0ad413f to
5616230
Compare
5616230 to
4ee3c1d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces tracking and alignment of the effective chunk size based on the server's chunk granularity during resumable uploads. It exposes getEffectiveChunkSize() in both ResumableUploadFuture and ResumableUploadProgress, and updates exception messages to include this information. Feedback suggests making the chunk size alignment more robust by returning the configured chunk size directly if the server's chunk granularity is less than or equal to zero, avoiding potential failures on backends that do not specify granularity.
| "Configured chunk size (%s bytes) must be at least the server chunk granularity (%s bytes)", | ||
| configuredChunkSize, | ||
| chunkGranularity); | ||
| int unalignedRemainder = (int) (configuredChunkSize % chunkGranularity); |
There was a problem hiding this comment.
(int) is a bit awkward, do we know if chunkGranularity from the server response is long or int? If we do, maybe we can serialize it to int?
There was a problem hiding this comment.
Over the wire it's not typed as long or int, but pragmatically it seems pretty much impossible that the server would prescribe a granularity expressible only as a long. Switched the lower-level types to parse the response into int and bubbled up the type change.
| checkArgument(configuredChunkSize > 0, "configuredChunkSize must be > 0"); | ||
| checkArgument(chunkGranularity > 0, "chunkGranularity must be > 0"); | ||
| checkArgument( | ||
| configuredChunkSize >= chunkGranularity, |
There was a problem hiding this comment.
Is this behavior a requirement? If not, maybe align the effective chunk size to be chunkGranularity if configuredChunkSize is smaller than it?
There was a problem hiding this comment.
Not a requirement AIUI and your suggestion makes more sense - done.
| if (t instanceof ApiException && uploadSessionUrl == null) { | ||
| String sessionUrl; | ||
| Integer chunkSize; | ||
| synchronized (lock) { |
There was a problem hiding this comment.
Do we need a synchronized block here and the getter below? volatile should be good enough since there should always be only one thread writing.
We don't have to always return them together as well, it's OK if one of them is null. It is also very unlikely that one of them is populated and the other one is null.
There was a problem hiding this comment.
The getter is needed because passing the chunk size is required for resuming, so we need to provide a way for the user to get it in case their provided value is changed for granularity reasons. From the requirements: "Since the upload resume does not start with the start command, there is no opportunity to receive the server-specified granularity and adjust the chunk size."
Unless you feel strongly, I prefer keeping the switch to synchronized. It was prompted by my investigating an earlier Gemini review finding - in general as I was digging into it the threading characteristics were IMO harder to reason about with multiple volatile fields mixed with others that legitimately require synchronization. I personally find it more readable/understandable to consolidate on synchronizing these even with a single writer (and I don't think this is a hot spot where synchronizing will cause performance issues).
|
|
||
| /** | ||
| * Returns the effective chunk size in bytes aligned to the server's chunk granularity, or {@code | ||
| * null} if session initiation is in progress. |
There was a problem hiding this comment.
nit: It can also be null if the session initiation failed. Same thing for getUploadSessionUrl above.
There was a problem hiding this comment.
Good catch, adjusted both docs.
…d surface effective chunk size
4ee3c1d to
43ec12a
Compare
|
|
X-Goog-Upload-Chunk-GranularityinResumableUploadFutureImplonce session initiation completes.ResumableUploadFuture.getEffectiveChunkSize(),ResumableUploadProgress.getEffectiveChunkSize(), and in terminalApiExceptionmessages alongside the upload URL.