From 2defe3559de8ad5d2f1488b12a401fe85532c41b Mon Sep 17 00:00:00 2001 From: Deependra Patel Date: Thu, 13 Jan 2022 13:05:30 +0530 Subject: [PATCH 1/5] Also retry on request timeout from server --- .../hadoop/gcsio/GoogleCloudStorageTest.java | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java b/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java index d134ad2c7..2c8a88942 100644 --- a/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java +++ b/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java @@ -123,6 +123,7 @@ public class GoogleCloudStorageTest { new StorageResourceId(BUCKET_NAME, OBJECT_NAME); private static final int STATUS_CODE_RESUME_INCOMPLETE = 308; + private static final int STATUS_CODE_REQUEST_TIMEOUT = 408; private static final ImmutableList ILLEGAL_OBJECTS = ImmutableList.copyOf( @@ -422,6 +423,67 @@ public void upload_failure_runtimeException() throws Exception { .inOrder(); } + @Test + public void upload_retry_requestTimeout() throws Exception { + byte[] testData = new byte[MediaHttpUploader.MINIMUM_CHUNK_SIZE]; + new Random().nextBytes(testData); + + MockHttpTransport transport = + mockTransport( + emptyResponse(HttpStatusCodes.STATUS_CODE_NOT_FOUND), + resumableUploadResponse(BUCKET_NAME, OBJECT_NAME), + emptyResponse(STATUS_CODE_REQUEST_TIMEOUT), + jsonDataResponse( + newStorageObject(BUCKET_NAME, OBJECT_NAME) + .setSize(BigInteger.valueOf(testData.length)))); + + GoogleCloudStorage gcs = mockedGcs(GCS_OPTIONS, transport); + + try (WritableByteChannel writeChannel = gcs.create(RESOURCE_ID)) { + writeChannel.write(ByteBuffer.wrap(testData)); + } + + assertThat(trackingRequestInitializerWithRetries.getAllRequestStrings()) + .containsExactly( + getRequestString(BUCKET_NAME, OBJECT_NAME), + resumableUploadRequestString( + BUCKET_NAME, OBJECT_NAME, /* generationId= */ 0, /* replaceGenerationId= */ false), + resumableUploadChunkRequestString(BUCKET_NAME, OBJECT_NAME, /* uploadId= */ 1), + resumableUploadChunkRequestString(BUCKET_NAME, OBJECT_NAME, /* uploadId= */ 2)) + .inOrder(); + } + + @Test + public void upload_does_not_retry_forbidden() throws Exception { + byte[] testData = new byte[MediaHttpUploader.MINIMUM_CHUNK_SIZE]; + new Random().nextBytes(testData); + + MockHttpTransport transport = + mockTransport( + emptyResponse(HttpStatusCodes.STATUS_CODE_NOT_FOUND), + resumableUploadResponse(BUCKET_NAME, OBJECT_NAME), + emptyResponse(HttpStatusCodes.STATUS_CODE_FORBIDDEN), + jsonDataResponse( + newStorageObject(BUCKET_NAME, OBJECT_NAME) + .setSize(BigInteger.valueOf(testData.length)))); + + GoogleCloudStorage gcs = mockedGcs(GCS_OPTIONS, transport); + + WritableByteChannel writeChannel = gcs.create(RESOURCE_ID); + writeChannel.write(ByteBuffer.wrap(testData)); + + IOException thrown = assertThrows(IOException.class, writeChannel::close); + assertThat(thrown).hasCauseThat().hasMessageThat().contains(String.valueOf(HttpStatusCodes.STATUS_CODE_FORBIDDEN)); + + assertThat(trackingRequestInitializerWithRetries.getAllRequestStrings()) + .containsExactly( + getRequestString(BUCKET_NAME, OBJECT_NAME), + resumableUploadRequestString( + BUCKET_NAME, OBJECT_NAME, /* generationId= */ 0, /* replaceGenerationId= */ false), + resumableUploadChunkRequestString(BUCKET_NAME, OBJECT_NAME, /* uploadId= */ 1)) + .inOrder(); + } + @Test public void reupload_success_singleWrite_multipleUploadChunks() throws Exception { byte[] testData = new byte[2 * MediaHttpUploader.MINIMUM_CHUNK_SIZE]; From e7ebbd7d40bc5df19e3e430ef64536180bc86b60 Mon Sep 17 00:00:00 2001 From: Deependra Patel Date: Thu, 13 Jan 2022 13:05:30 +0530 Subject: [PATCH 2/5] spotfix --- .../google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java b/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java index 2c8a88942..2f12d5836 100644 --- a/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java +++ b/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java @@ -473,7 +473,10 @@ public void upload_does_not_retry_forbidden() throws Exception { writeChannel.write(ByteBuffer.wrap(testData)); IOException thrown = assertThrows(IOException.class, writeChannel::close); - assertThat(thrown).hasCauseThat().hasMessageThat().contains(String.valueOf(HttpStatusCodes.STATUS_CODE_FORBIDDEN)); + assertThat(thrown) + .hasCauseThat() + .hasMessageThat() + .contains(String.valueOf(HttpStatusCodes.STATUS_CODE_FORBIDDEN)); assertThat(trackingRequestInitializerWithRetries.getAllRequestStrings()) .containsExactly( From 3df3fc7cd18af2e5862f7a99e8e7f8bb5ad3f4e0 Mon Sep 17 00:00:00 2001 From: Deependra Patel Date: Tue, 18 Jan 2022 22:10:34 +0530 Subject: [PATCH 3/5] address comments --- .../com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java b/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java index 2f12d5836..c3d00712c 100644 --- a/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java +++ b/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java @@ -454,7 +454,7 @@ public void upload_retry_requestTimeout() throws Exception { } @Test - public void upload_does_not_retry_forbidden() throws Exception { + public void upload_noRetries_forbidden() throws Exception { byte[] testData = new byte[MediaHttpUploader.MINIMUM_CHUNK_SIZE]; new Random().nextBytes(testData); From 1835595f740d1490461da2f660058ae2f0ee9ce2 Mon Sep 17 00:00:00 2001 From: Deependra Patel Date: Tue, 18 Jan 2022 22:13:40 +0530 Subject: [PATCH 4/5] address comments 2 --- .../com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java b/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java index c3d00712c..f3a1fdac2 100644 --- a/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java +++ b/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java @@ -34,6 +34,7 @@ import static com.google.cloud.hadoop.gcsio.TrackingHttpRequestInitializer.resumableUploadChunkRequestString; import static com.google.cloud.hadoop.gcsio.TrackingHttpRequestInitializer.resumableUploadRequestString; import static com.google.cloud.hadoop.gcsio.TrackingHttpRequestInitializer.uploadRequestString; +import static com.google.cloud.hadoop.util.RetryHttpInitializer.HTTP_REQUEST_TIMEOUT; import static com.google.cloud.hadoop.util.testing.MockHttpTransportHelper.dataResponse; import static com.google.cloud.hadoop.util.testing.MockHttpTransportHelper.emptyResponse; import static com.google.cloud.hadoop.util.testing.MockHttpTransportHelper.inputStreamResponse; @@ -123,7 +124,6 @@ public class GoogleCloudStorageTest { new StorageResourceId(BUCKET_NAME, OBJECT_NAME); private static final int STATUS_CODE_RESUME_INCOMPLETE = 308; - private static final int STATUS_CODE_REQUEST_TIMEOUT = 408; private static final ImmutableList ILLEGAL_OBJECTS = ImmutableList.copyOf( @@ -432,7 +432,7 @@ public void upload_retry_requestTimeout() throws Exception { mockTransport( emptyResponse(HttpStatusCodes.STATUS_CODE_NOT_FOUND), resumableUploadResponse(BUCKET_NAME, OBJECT_NAME), - emptyResponse(STATUS_CODE_REQUEST_TIMEOUT), + emptyResponse(HTTP_REQUEST_TIMEOUT), jsonDataResponse( newStorageObject(BUCKET_NAME, OBJECT_NAME) .setSize(BigInteger.valueOf(testData.length)))); From 99ff8ba976c3cab51a7e57ebbb3969782e1ebdbe Mon Sep 17 00:00:00 2001 From: Deependra Patel Date: Wed, 10 Aug 2022 14:02:20 +0530 Subject: [PATCH 5/5] resolve merge conflicts --- .../cloud/hadoop/gcsio/GoogleCloudStorageTest.java | 3 +-- .../cloud/hadoop/util/RetryHttpInitializer.java | 12 +++++++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java b/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java index f3a1fdac2..798b7476a 100644 --- a/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java +++ b/gcsio/src/test/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageTest.java @@ -34,7 +34,6 @@ import static com.google.cloud.hadoop.gcsio.TrackingHttpRequestInitializer.resumableUploadChunkRequestString; import static com.google.cloud.hadoop.gcsio.TrackingHttpRequestInitializer.resumableUploadRequestString; import static com.google.cloud.hadoop.gcsio.TrackingHttpRequestInitializer.uploadRequestString; -import static com.google.cloud.hadoop.util.RetryHttpInitializer.HTTP_REQUEST_TIMEOUT; import static com.google.cloud.hadoop.util.testing.MockHttpTransportHelper.dataResponse; import static com.google.cloud.hadoop.util.testing.MockHttpTransportHelper.emptyResponse; import static com.google.cloud.hadoop.util.testing.MockHttpTransportHelper.inputStreamResponse; @@ -432,7 +431,7 @@ public void upload_retry_requestTimeout() throws Exception { mockTransport( emptyResponse(HttpStatusCodes.STATUS_CODE_NOT_FOUND), resumableUploadResponse(BUCKET_NAME, OBJECT_NAME), - emptyResponse(HTTP_REQUEST_TIMEOUT), + emptyResponse(408), // HTTP 408 Request Timeout jsonDataResponse( newStorageObject(BUCKET_NAME, OBJECT_NAME) .setSize(BigInteger.valueOf(testData.length)))); diff --git a/util/src/main/java/com/google/cloud/hadoop/util/RetryHttpInitializer.java b/util/src/main/java/com/google/cloud/hadoop/util/RetryHttpInitializer.java index 5b80069bc..15c8d62f4 100644 --- a/util/src/main/java/com/google/cloud/hadoop/util/RetryHttpInitializer.java +++ b/util/src/main/java/com/google/cloud/hadoop/util/RetryHttpInitializer.java @@ -35,6 +35,7 @@ import com.google.common.flogger.GoogleLogger; import com.google.common.flogger.LogContext; import java.io.IOException; +import java.util.Set; /** An implementation of {@link HttpRequestInitializer} with retries. */ public class RetryHttpInitializer implements HttpRequestInitializer { @@ -116,6 +117,15 @@ private static class UnsuccessfulResponseHandler implements HttpUnsuccessfulResp /** HTTP status code indicating too many requests in a given amount of time. */ private static final int HTTP_SC_TOO_MANY_REQUESTS = 429; + /** + * HTTP status code indicating that the server has decided to close the connection rather than + * continue waiting + */ + private static final int HTTP_REQUEST_TIMEOUT = 408; + + private static final Set RETRYABLE_CODES = + ImmutableSet.of(HTTP_SC_TOO_MANY_REQUESTS, HTTP_REQUEST_TIMEOUT); + // The set of response codes to log URLs for with a rate limit. private static final ImmutableSet RESPONSE_CODES_TO_LOG_WITH_RATE_LIMIT = ImmutableSet.of(HTTP_SC_TOO_MANY_REQUESTS); @@ -132,7 +142,7 @@ private static class UnsuccessfulResponseHandler implements HttpUnsuccessfulResp // of the bases cases defined by this instance. private static final HttpBackOffUnsuccessfulResponseHandler.BackOffRequired BACK_OFF_REQUIRED = response -> - response.getStatusCode() == HTTP_SC_TOO_MANY_REQUESTS + RETRYABLE_CODES.contains(response.getStatusCode()) || HttpBackOffUnsuccessfulResponseHandler.BackOffRequired.ON_SERVER_ERROR .isRequired(response);