Conversation
![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.
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
|
|
2 participants
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.