Conversation
f7dacda to
ea2e309
Compare
There was a problem hiding this comment.
Code Review
This pull request implements Category 2 (recoverable) error recovery for resumable uploads in GAX Java, updating ChunkAttemptCallable to query session status, realign the buffer window, and dispatch chunk uploads upon encountering recoverable errors or missing status headers. The review feedback highlights critical improvement opportunities in ChunkAttemptCallable, including addressing a potential infinite loop by correctly classifying query status failures under UploadCommand.QUERY rather than UPLOAD, and preventing race conditions by checking if the retrying future is cancelled immediately after setting inFlightFuture for both query and chunk upload futures.
| public void onFailure(Throwable t) { | ||
| failAttempt(attemptFuture, t); | ||
| } |
There was a problem hiding this comment.
If the query status call fails with a non-transient error (such as 400 Bad Request or 403 Forbidden), classifying it under the chunk's upload command (e.g., UPLOAD) in UploadResultRetryAlgorithm would incorrectly classify it as RECOVERABLE (since 400 is recoverable for UPLOAD). This would trigger another recovery attempt, leading to an infinite loop of failing query status calls. We should classify the query failure using UploadCommand.QUERY and fail the attempt with a non-retryable exception if it is not transient.
@Override
public void onFailure(Throwable t) {
UploadErrorCategory category = UploadErrorClassifier.classify(t, UploadCommand.QUERY);
if (category == UploadErrorCategory.TRANSIENT) {
failAttempt(attemptFuture, t);
} else {
failAttempt(
attemptFuture,
new IllegalStateException("Fatal error during query status: " + t.getMessage(), t));
}
}| attemptFuture, new IllegalStateException("queryStatusCallable returned a null future")); | ||
| return; | ||
| } | ||
| this.inFlightFuture = queryFuture; |
There was a problem hiding this comment.
To prevent a race condition where the retrying future is cancelled concurrently before inFlightFuture is set, we should check if currentRetryingFuture is cancelled immediately after setting inFlightFuture and propagate the cancellation if so.
this.inFlightFuture = queryFuture;
if (currentRetryingFuture.isCancelled()) {
queryFuture.cancel(true);
}|
|
||
| ApiFuture<ChunkUploadResponse<ResponseT>> chunkFuture = | ||
| uploadChunkCallable.futureCall(currentRequest, attemptContext); | ||
| this.inFlightFuture = chunkFuture; |
There was a problem hiding this comment.
To prevent a race condition where the retrying future is cancelled concurrently before inFlightFuture is set, we should check if currentRetryingFuture is cancelled immediately after setting inFlightFuture and propagate the cancellation if so.
this.inFlightFuture = chunkFuture;
if (currentRetryingFuture.isCancelled()) {
chunkFuture.cancel(true);
}ea2e309 to
e197ee8
Compare
e197ee8 to
12cd1cb
Compare
12cd1cb to
dd1aaac
Compare
dd1aaac to
7fb2335
Compare
7fb2335 to
4795631
Compare
Introduces ChunkAttemptCallable with a query-and-realign loop when encountering recoverable protocol errors. Queries the server for the committed offset and adjusts the buffer window before resuming chunk transmission.
4795631 to
1fe75dc
Compare
|
|




Introduces
ChunkAttemptCallablewith a query-and-realign loop when encountering recoverable protocol errors. Queries the server for the committed offset and adjusts the buffer window before resuming chunk transmission.