feat(gax): retry chunk upload on transient errors - #14422
Conversation
809615f to
d186a68
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
d186a68 to
cbbeb54
Compare
7c209f2 to
ce8a9f5
Compare
ce8a9f5 to
896cd2f
Compare
41d94b6 to
6b71690
Compare
6b71690 to
0f8b144
Compare
0f8b144 to
d1e3b92
Compare
79b57f6 to
f2e3c65
Compare
f2e3c65 to
18e2288
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces automatic chunk retries with exponential backoff for resumable uploads by wrapping the chunk upload callable in a RetryingCallable and executing chunk transmissions sequentially. It also adds comprehensive unit tests to verify retry behavior, exhaustion, and cancellation. The review feedback highlights a security concern regarding the inclusion of the full uploadUrl in an exception message, as it may contain sensitive session tokens that could be leaked in logs.
| result.setException( | ||
| new IllegalStateException( | ||
| "Upload stream ended and final chunk was transmitted, but server returned" | ||
| + " incomplete status")); | ||
| + " incomplete status for upload URL: " | ||
| + uploadUrl)); |
There was a problem hiding this comment.
Including the full uploadUrl in the exception message poses a security risk. Resumable upload URLs contain sensitive session IDs or tokens (such as upload_id in Google Cloud Storage) that act as bearer credentials. If this exception is logged or propagated to client applications, it could leak these credentials. Please revert to the original exception message or redact the sensitive query parameters from the URL before including it in the exception.
result.setException(
new IllegalStateException(
"Upload stream ended and final chunk was transmitted, but server returned"
+ " incomplete status"));There was a problem hiding this comment.
Leaving the URL in - it's a requirement that the URL is included in failure messages when possible. (I actually have a subsequent PR which expands that behavior).
506f1b7 to
ea6839b
Compare
ea6839b to
d7d1947
Compare
d7d1947 to
08e807a
Compare
| extends ResumableUploadCallable<RequestT, ResponseT> { | ||
|
|
||
| private static final RetrySettings RETRY_SETTINGS = | ||
| RetrySettings.newBuilder() |
There was a problem hiding this comment.
We should at least set initialRpcTimeoutDuration and totalTimeoutDuration. Otherwise the call could hang indefinitely (before global timeout kicks in). These are the default generated retrySetting values.
Separately, check if there is a cross-language sensible default.
There was a problem hiding this comment.
The cross-language docs aren't very prescriptive here - no suggested values that I could see.
I did take a look at the Python resumable upload implementation and set initialRpcTimeoutDuration to 1m and totalTimeoutDuration to 2m as a result. I also adjusted the backoff params to mirror Python's too (now 1s initial delay, 2x multiplier, 1m max delay) - the former settings I had looked like they'd exhaust the retries in < 1s if the wire commands were failing fast.
|
|
||
| // Serializes buffer mutations across multiple threads (i.e. from retry/recovery) | ||
| private final Executor chunkExecutor = | ||
| MoreExecutors.newSequentialExecutor(MoreExecutors.directExecutor()); |
There was a problem hiding this comment.
Is this to prevent stackoverflow of the sequential transmitChunk call? If it is, I don't think it would happen because each call will be run in a separate IOExecutor thread.
There was a problem hiding this comment.
Yeah, IIRC a Gemini review pointed out the overflow risk - but I think you're right, other than artificial executors in unit tests the calls will go on IO threads. Removed this.
|
|
||
| private static final byte[] EMPTY_PAYLOAD = new byte[0]; | ||
|
|
||
| // Serializes buffer mutations across multiple threads (i.e. from retry/recovery) |
There was a problem hiding this comment.
I don't think there is a case for "serializing buffer mutations across multiple threads"? retry/recovery is always sequential.
There was a problem hiding this comment.
Removed this executor.
08e807a to
4257f81
Compare
6b6d432 to
920d662
Compare
a456ecb to
6d6f62a
Compare
| throw new UnsupportedOperationException("Session resumption is not yet implemented."); | ||
| } | ||
|
|
||
| private static <ReqT, RespT> UnaryCallable<ReqT, RespT> createRetryingCallable( |
There was a problem hiding this comment.
nit: use <RequestT, ResponseT> to be consistent with the class.
6d6f62a to
de5ae9a
Compare
Wraps chunk uploads in a retrying executor to retry transient network and server errors using exponential backoff. Retries individual chunks without restarting the entire upload session.
de5ae9a to
5f8515f
Compare
|
|
🤖 I have created a release *beep* *boop* --- <details><summary>1.93.0</summary> ## [1.93.0](v1.92.0...v1.93.0) (2026-09-30) ### ⚠ BREAKING CHANGES * **automl:** remove java-automl library ([#14540](#14540)) ### Features * **automl:** remove java-automl library ([#14540](#14540)) ([6f13a1d](6f13a1d)) * **bigquery-jdbc:** infer undeclared fields in PreparedStatement ([#14400](#14400)) ([e989a72](e989a72)) * **gax,gapic-generator-java:** retry start upload call and rename ResumableUploadCallSettings to ResumableUploadOptions ([#14511](#14511)) ([98c2073](98c2073)) * **gax:** add chunk upload recovery loop for resumable uploads ([#14424](#14424)) ([8ecd1e5](8ecd1e5)) * **gax:** add ResumableUploadProgressListener and related infrastructure ([#14426](#14426)) ([c7f10d8](c7f10d8)) * **gax:** add RewindableStreamBuffer in prep for chunk upload recovery ([#14423](#14423)) ([349c315](349c315)) * **gax:** enforce global timeout for resumable uploads ([#14425](#14425)) ([3cb9805](3cb9805)) * **gax:** include upload-status header in resumable upload command response objects ([#14420](#14420)) ([23e49c1](23e49c1)) * **gax:** replace InputStream with InputStreamSupplier in resumable upload public surfaces ([#14521](#14521)) ([3c79730](3c79730)) * **gax:** retry chunk upload on transient errors ([#14422](#14422)) ([e72365e](e72365e)) * **gax:** treat resumable upload server rejection as terminal ([#14516](#14516)) ([b288788](b288788)) * **gax:** wire up ResumableUploadProgressTracker ([#14427](#14427)) ([e7ba3e8](e7ba3e8)) * **grpc-gcp:** Add shared fallback state and probing recovery options to GcpFallbackChannel ([#14013](#14013)) ([675f639](675f639)) * **pubsub:** add publish start time to client telemetry header ([#14496](#14496)) ([4b6a671](4b6a671)) ### Bug Fixes * **auth:** exclude javax.annotation-api from api-common dependency ([#14538](#14538)) ([668fd18](668fd18)), refs [#12363](#12363) * **bigquery-jdbc:** abort session when connection is closed ([#14273](#14273)) ([5bd4520](5bd4520)), refs [#13922](#13922) * **storage:** add App Hub storage.googleapis.com prefix to destination.id ([#14527](#14527)) ([0f6f9b5](0f6f9b5)) ### Documentation * Add gRPC Post-Quantum Cryptography Guide ([#14245](#14245)) ([340a239](340a239)) * **samples:** add zonal bucket pre-warmed writer pool sample ([#14517](#14517)) ([32e6f87](32e6f87)) * **samples:** pre-warm writer pool channels with flush() after open() ([#14537](#14537)) ([df06290](df06290)) * update PQC guide to follow standard template format ([#14333](#14333)) ([0b969d5](0b969d5)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
2 participants
Wraps chunk uploads in a
RetryingCallableto retry transient errors.Note: the retry settings used here are intentionally different from those that will be used for starting the upload; our requirement for this milestone is that "the retry policy [specified by the user] is only applicable to the initial request."