feat(gax): add resumable upload error classification and retry algorithm - #14419
Conversation
ebf651d to
58cb833
Compare
a6995e8 to
a2b7fd6
Compare
a2b7fd6 to
2419e04
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a resumable upload error classification mechanism to GAX, adding the ResumableUploadCommand enum, the ResumableUploadErrorClassifier to categorize exceptions (as transient, recoverable, or fatal), and the ResumableUploadResultRetryAlgorithm to integrate with GAX's retry logic, along with comprehensive unit tests. The review feedback points out a potential risk in ResumableUploadErrorClassifier where querying HTTP_STATUS_MAP with statusCode.getTransportCode() could lead to a NullPointerException or type mismatch, and suggests adding an explicit instanceof Integer check to ensure robustness.
| return Category.FATAL; | ||
| } | ||
|
|
||
| Category category = HTTP_STATUS_MAP.getOrDefault(statusCode.getTransportCode(), Category.FATAL); |
There was a problem hiding this comment.
Since StatusCode.getTransportCode() returns Object, it can potentially return null or a non-Integer type (e.g., in non-HTTP transports or uninitialized states). Querying an ImmutableMap with a null key or an incompatible type is risky and can lead to unexpected behavior or NullPointerException depending on the map implementation. It is safer and more robust to perform an explicit instanceof Integer check before looking up the status code in HTTP_STATUS_MAP.
| Category category = HTTP_STATUS_MAP.getOrDefault(statusCode.getTransportCode(), Category.FATAL); | |
| Object transportCode = statusCode.getTransportCode(); | |
| Category category = Category.FATAL; | |
| if (transportCode instanceof Integer) { | |
| category = HTTP_STATUS_MAP.getOrDefault((Integer) transportCode, Category.FATAL); | |
| } |
There was a problem hiding this comment.
Using getOrDefault should handle both null and non-Integer keys safely.
2419e04 to
6f082cf
Compare
|
|
||
| private static boolean isRecoverableCommand(ResumableUploadCommand command) { | ||
| switch (command) { | ||
| case UPLOAD: |
There was a problem hiding this comment.
We can add an isRecoverable field to the command enum. Same thing for the classifyMissingStatusHeader above if we need it.
There was a problem hiding this comment.
Great suggestion - done.
|
|
||
| // HttpJsonApiExceptionFactory wraps non-HTTP errors as UNKNOWN. | ||
| if (statusCode.getCode() == StatusCode.Code.UNKNOWN) { | ||
| if (apiException.getCause() instanceof IOException) { |
There was a problem hiding this comment.
Is this for client side exceptions such as unable to open a filestream?
There was a problem hiding this comment.
No, it's not the plan to have wholly client side failures like reading the InputStream subject to this retry classification (i.e. via ResumableUploadResultRetryAlgorithm.) The requirements doc specifies that those should fail fast.
IOExceptions caught here would be something like network failures, which the requirements dictate should be considered transient.
There was a problem hiding this comment.
The requirements doc specifies that those should fail fast.
I agree. Is this already implemented?
IOExceptions caught here would be something like network failures
I'm a little worried that it maybe too broad. For example, an UnknownHostException might be from no internet or misconfigured endpoint. Do we have a list of retryable IOExceptions?
There was a problem hiding this comment.
Is this already implemented?
Yes, stream reading exceptions fail the entire session when caught. The exact shape of this will change over the next few PRs with non-happy path features layered on, but it will continue to fail fast here.
I'm a little worried that it maybe too broad.
That makes sense - looking at the requirements, the exceptions enumerated as retriable other than those associated with specific HTTP codes are just "TCP/Socket Timeout". So I narrowed down to just SocketTimeoutException; if we need to expand the list we can adjust that in the future.
c36f9e0 to
c0d2d5e
Compare
| } | ||
| // Transient errors are retried directly with the identical request, others are not. | ||
| Category category = ResumableUploadErrorClassifier.classify(previousThrowable, command); | ||
| return category == TRANSIENT; |
There was a problem hiding this comment.
It seems only TRANSIENT is used now for retrying. I guess RECOVERABLE will be used for deciding if we should query and recover later?
There was a problem hiding this comment.
Correct. I've been having a bit of a back-and-forth on whether that will be captured in the retry algorithm in this milestone (maybe a topic for an offline discussion) but in either case the RECOVERABLE value is a marker for kicking off the retry/recovery handshake.
c0d2d5e to
3445d8e
Compare
Introduce UploadCommand, UploadErrorClassifier, and UploadResultRetryAlgorithm to classify HTTP response codes and transport-level exceptions during resumable upload sessions into protocol error categories (TRANSIENT, RECOVERABLE, FATAL). Implements the classification order: 1. CancellationException is terminal (FATAL) and never retried. 2. ApiException with StatusCode.Code.UNKNOWN unwraps cause. GAX wraps unrecognized runtime throwables into Code.UNKNOWN, which carries a synthetic HTTP 500 transport code. Without this explicit step, local bugs and NPEs would be misclassified as transient 500s and retried indefinitely. Real wire 500 responses arrive with Code.INTERNAL and are TRANSIENT. 3. Table lookup on raw HTTP transport code (408, 429, 500, 502, 503, 504 are TRANSIENT; 400, 409, 412, 416 are RECOVERABLE; 401, 403, 404, 405, 410, 413, 415 are FATAL). Note that wire 408 and 412 both map to FAILED_PRECONDITION under HttpJsonStatusCode, but diverge based on raw HTTP transport code. 4. Plain I/O or timeout exceptions that bypassed ApiException wrapping are TRANSIENT; anything else unrecognized is FATAL.
3445d8e to
4225876
Compare
|
|
🤖 I have created a release *beep* *boop* --- <details><summary>1.92.0</summary> ## [1.92.0](v1.91.0...v1.92.0) (2026-09-23) ### Features * **bigquery-jdbc:** add `EnableTimestampPicos` connection property and its plumbing ([#14284](#14284)) ([b4aa5ac](b4aa5ac)) * **bigquery-jdbc:** implement picosecond temporal math and formatting engine ([#14286](#14286)) ([2a9612a](2a9612a)) * **bigquery-jdbc:** support picosecond in REST JSON path and nested types ([#14334](#14334)) ([15ffe4a](15ffe4a)) * **bigquery-jdbc:** support picosecond in `PreparedStatement` parameters and batching ([#14373](#14373)) ([c1aac66](c1aac66)) * **bigquery-jdbc:** support picosecond timestamp in `ResultSetMetaData` and `DatabaseMetaData` ([#14358](#14358)) ([43acdd3](43acdd3)) * **bigquery-jdbc:** support picosecond timestamps in Arrow Storage Read API and nested types ([#14332](#14332)) ([b5d9aca](b5d9aca)) * **bigquery-jdbc:** support qualified project delimiter in `DefaultDataset` property ([#14240](#14240)) ([6e8d6c8](6e8d6c8)) * **bigquery:** accelerate row-based query() with Arrow wire format ([#14405](#14405)) ([8d12a8f](8d12a8f)) * **bigquery:** add ArrowDeserializer helper utility ([#13943](#13943)) ([d9a298b](d9a298b)) * **bigquery:** add ArrowQueryPageFetcher for Arrow query result pagination ([#14404](#14404)) ([615409f](615409f)) * **bigquery:** add ArrowQueryResult and ArrowQueryResultImpl for Arrow result streaming ([#13944](#13944)) ([a62fdf8](a62fdf8)) * **bigquery:** add Storage Read API slow-path fallback for row-based query() ([#14409](#14409)) ([26e568a](26e568a)) * **bigquery:** add zero-copy queryArrow API for Arrow VectorSchemaRoot streaming ([#14402](#14402)) ([b44ffe8](b44ffe8)) * **bigquery:** make BigQuery AutoCloseable with default no-op close method ([#14434](#14434)) ([00bf3de](00bf3de)) * **firestore:** add support for BSON types ([#13189](#13189)) ([8a123d9](8a123d9)) * **gax:** add ApiCallContext and request-level settings overloads to ResumableUploadCallable ([#14251](#14251)) ([e8cbd42](e8cbd42)) * **gax:** add globalTimeout settings field to ResumableUploadCallSettings ([#14253](#14253)) ([438cda6](438cda6)) * **gax:** add resumable upload error classification and retry algorithm ([#14419](#14419)) ([b70396d](b70396d)) * **gax:** add ResumableUploadCallable creation to Callables and HttpJsonCallableFactory ([#14242](#14242)) ([7de24de](7de24de)) * **gax:** implement baseline Callable and Future for resumable uploads ([#14241](#14241)) ([5a54db9](5a54db9)) * **generator:** add model flag and allowlist parser for resumable upload RPCs ([#14317](#14317)) ([acc1856](acc1856)) * **generator:** emit resumable upload client surface ([#14319](#14319)) ([a9fed00](a9fed00)) * **generator:** emit resumable upload settings and HttpJson upload stub ([#14321](#14321)) ([c122474](c122474)) * **generator:** enable resumable upload generation for showcase ([#14325](#14325)) ([f9ebd79](f9ebd79)) * **generator:** switch resumable upload specialized stubs to package private ([#14471](#14471)) ([0d4e875](0d4e875)) * **generator:** wire transport stub delegation to resumable upload stubs ([#14322](#14322)) ([cc4b980](cc4b980)) * **google/cloud/backupdr/v1beta:** add backupdr ([#14410](#14410)) ([a4a47da](a4a47da)) * **google/cloud/networkservices/v1beta1:** add networkservices ([#14407](#14407)) ([21c4955](21c4955)) * **pubsub:** add publish telemetry headers for publish attempt observability ([#14338](#14338)) ([c167ab8](c167ab8)) * **pubsub:** implement publish hedging to reduce tail latency ([#13735](#13735)) ([b302615](b302615)) * **spanner:** Support dynamic TLS certificate and key rotation for Spanner Omni ([#14456](#14456)) ([ffc745c](ffc745c)) * **storage/control:** add delete folder recursive sample ([#13642](#13642)) ([f4b1b46](f4b1b46)) * **storage/control:** add delete folder recursive sample ([#14397](#14397)) ([2c01d55](2c01d55)) ### Bug Fixes * **auth:** restore transportFactory upon deserialization in InternalAwsSecurityCredentialsSupplier ([#14340](#14340)) ([beea42f](beea42f)) * **bigquery-jdbc:** ensure row ordering in PCNT IT ([#14330](#14330)) ([a16f048](a16f048)) * **bigquery-jdbc:** fix htapi fallback due to permission logic ([#14418](#14418)) ([21e6dc8](21e6dc8)) * **bigquery-jdbc:** fix Timestamp assertions ([#14290](#14290)) ([533ba14](533ba14)) * **bigquery-jdbc:** handle null parameters in Storage Write API bulk inserts ([#14270](#14270)) ([dd2c41a](dd2c41a)), refs [#14066](#14066) * **bigquery-jdbc:** handle SQL NULLs in ResultSet primitive getters ([#14383](#14383)) ([8e464fe](8e464fe)), refs [#14371](#14371) * **bigquery:** default Arrow pagination stream location to US instead of global ([#14458](#14458)) ([2775eb1](2775eb1)) * **bigquery:** preserve page token and paginate correctly in Arrow query when maxResults is set ([#14469](#14469)) ([f5601f4](f5601f4)) * **bigquery:** use first page row count for Arrow query pagination offset ([#14466](#14466)) ([9d10dd0](9d10dd0)) * **bigtable:** don't notify config listeners while holding the manager lock ([#14294](#14294)) ([4426ccd](4426ccd)) * **bigtable:** fall back to classic path when per-RPC CallCredentials are set on session path ([#14477](#14477)) ([57bacb0](57bacb0)) * **bigtable:** fix abnormal session closures and scale-up in session pool ([#14431](#14431)) ([6361ecd](6361ecd)) * **biqguery:** fix undeclared QueryParameter wiring in QueryStatistics ([#14401](#14401)) ([64cf1d3](64cf1d3)) * **bom:** restore google-cloud-spanner-jdbc to libraries-bom ([#14362](#14362)) ([bc7be5e](bc7be5e)), refs [#14347](#14347) * **spanner:** honor maxAttempts and totalTimeout in streaming resume loop ([#14370](#14370)) ([305f47d](305f47d)) * **spanner:** only set snapshot isolation read timestamp for SI or optimistic txns in CloudClientExecutor ([#14346](#14346)) ([54c0d0f](54c0d0f)) * **spanner:** prevent statement cancellation race in AbstractBaseUnitOfWork ([#14283](#14283)) ([d9a8eef](d9a8eef)) * **spanner:** re-enable ITInstanceAdminTest on cloud-devel and cloud-staging ([#14281](#14281)) ([89a8268](89a8268)) ### Performance Improvements * **spanner:** stop re-parsing the request id on every RPC ([#14353](#14353)) ([46108f4](46108f4)) ### Documentation * Add a Http/Json Post-Quantum Cryptography Guide ([#13963](#13963)) ([fcc65b0](fcc65b0)) * **bigquery:** add QueryArrow code sample and document JDK 17+ JVM requirements ([#14437](#14437)) ([bd363f6](bd363f6)) </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>




The intent is to centralize the logic that determines the appropriate action (fail fast, retry, chunk recovery) for the various types/codes of errors that a resumable upload operation might encounter. Since the requirements for which error should trigger which action during which operation are quite complex it's helpful for the logic to be consolidated in one place.