Conversation
c150a47 to
755a631
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.
755a631 to
aed4f4b
Compare
aed4f4b to
2f4434b
Compare
2f4434b to
445db61
Compare
445db61 to
86a18dd
Compare
429933c to
628d442
Compare
whowes
changed the title
whowes
changed the title
628d442 to
4ee20f2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request replaces the boolean 'isComplete' flag in 'ChunkUploadResponse' and 'QueryStatusResponse' with a new 'ResumableUploadStatus' enum to represent the upload session status ('ACTIVE', 'FINAL', 'CANCELLED', 'UNKNOWN'). It updates the chunk and query status callables, improves error handling for cancelled or rejected uploads, and updates the corresponding tests. The review feedback suggests simplifying the creation of 'FAILED_PRECONDITION_STATUS_CODE' by using the existing 'HttpJsonStatusCode' class instead of an anonymous class, and combining the high-level context message with the cause's message in 'createServerRejectionException' to preserve debugging details.
| private static final StatusCode FAILED_PRECONDITION_STATUS_CODE = | ||
| new StatusCode() { | ||
| @Override | ||
| public Code getCode() { | ||
| return Code.FAILED_PRECONDITION; | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable Integer getTransportCode() { | ||
| return null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Instead of creating a verbose anonymous class implementing StatusCode, you can use the existing HttpJsonStatusCode class which is already available in this package. This simplifies the code and improves maintainability.
private static final StatusCode FAILED_PRECONDITION_STATUS_CODE =
HttpJsonStatusCode.of(StatusCode.Code.FAILED_PRECONDITION);There was a problem hiding this comment.
This would cause a bug - HttpJsonStatusCode.of sets the transport code to 400, which would cause this to be classified as recoverable (it should not be).
| static Throwable createServerRejectionException( | ||
| int statusCode, ResumableUploadStatus uploadStatus, HttpJsonMetadata trailers) { | ||
| Throwable cause = trailers.getException(); | ||
| String message = "Upload " + uploadStatus + " by server with status code: " + statusCode; | ||
| if (cause != null && cause.getMessage() != null) { | ||
| message = cause.getMessage(); | ||
| } | ||
| return ApiExceptionFactory.createException( | ||
| message, cause, FAILED_PRECONDITION_STATUS_CODE, /* retryable= */ false); | ||
| } |
There was a problem hiding this comment.
When creating the server rejection exception, if a cause with a message is present, the current implementation completely overwrites the high-level context message (which contains the uploadStatus and statusCode). It is better to append or combine the cause's message to preserve both the high-level context and the low-level error details for better debuggability.
| static Throwable createServerRejectionException( | |
| int statusCode, ResumableUploadStatus uploadStatus, HttpJsonMetadata trailers) { | |
| Throwable cause = trailers.getException(); | |
| String message = "Upload " + uploadStatus + " by server with status code: " + statusCode; | |
| if (cause != null && cause.getMessage() != null) { | |
| message = cause.getMessage(); | |
| } | |
| return ApiExceptionFactory.createException( | |
| message, cause, FAILED_PRECONDITION_STATUS_CODE, /* retryable= */ false); | |
| } | |
| static Throwable createServerRejectionException( | |
| int statusCode, ResumableUploadStatus uploadStatus, HttpJsonMetadata trailers) { | |
| Throwable cause = trailers.getException(); | |
| String message = "Upload " + uploadStatus + " by server with status code: " + statusCode; | |
| if (cause != null && cause.getMessage() != null) { | |
| message += ": " + cause.getMessage(); | |
| } | |
| return ApiExceptionFactory.createException( | |
| message, cause, FAILED_PRECONDITION_STATUS_CODE, /* retryable= */ false); | |
| } |
d4c96e9 to
eb683df
Compare
eb683df to
24a9298
Compare
24a9298 to
39ecb75
Compare
| future.set(chunkResponseBuilder.build()); | ||
| future.set(ChunkUploadResponse.create(uploadStatus, response)); | ||
| } else if (uploadStatus == ResumableUploadStatus.CANCELLED | ||
| || uploadStatus == ResumableUploadStatus.FINAL) { |
There was a problem hiding this comment.
This scenario is for statusCode != 2xx && uploadStatus == ResumableUploadStatus.FINAL?
Separately, I know we are not planning to implement cancel yet, is it possible for the server to return a CANCELLED state? Otherwise adding CANCELLED here makes the logic hard to read
There was a problem hiding this comment.
Correct; this is the "rejection flow" referred to in the cross-language design doc.
Removed CANCELLED state altogether for now - I verified that the server won't return that status unless the client requests cancellation, and we are not implementing that request in this milestone.
| message += ": " + cause.getMessage(); | ||
| } | ||
| return ApiExceptionFactory.createException( | ||
| message, cause, FAILED_PRECONDITION_STATUS_CODE, /* retryable= */ false); |
There was a problem hiding this comment.
What is the precondition that is failing here?
There was a problem hiding this comment.
What I was going for was to communicate non-retriability (documentation that was influencing me) but with a fresh look it's more actionable/debuggable for the user to propagate the code from the server. The transport code being null is sufficient for the classifier to classify as FATAL.
39ecb75 to
03cf152
Compare
Add nullable getUploadStatus() accessors to ChunkUploadResponse, QueryStatusResponse, and ResumableUploadSession, and plumb the X-Goog-Upload-Status response header through the HTTP/JSON callables. On HTTP 200 chunk responses where X-Goog-Upload-Status is absent, return a ChunkUploadResponse with a null uploadStatus rather than throwing a wire-level exception, allowing higher-level upload coordinators to classify the missing header and trigger protocol recovery. Existing test uploadChunk_missingUploadStatusHeader_throwsInternalException was updated to uploadChunk_missingUploadStatusHeader_returnsNullUploadStatusOnHttp200 to reflect that the missing status header on HTTP 200 is now surfaced via a null status property on ChunkUploadResponse instead of throwing an InternalException at the transport layer. Note: No end-to-end integration test is included because the test server always returns the X-Goog-Upload-Status header on success, making header absence uninjectable end-to-end.
03cf152 to
604405a
Compare
|
|
2 participants
This is needed to meet the requirement that the protocol implementation inspect responses for the presence of this header, responding differently in its absence depending on the command (e.g. treat as transient error for start commands, recoverable error for upload, fatal error for query)