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
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.
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); | |
| } |
|
|
1 participant
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.