Retry request on 408/timeout response from server

[Deependra-Patel]

No description provided.

[Deependra-Patel]

/gcbrun

[Deependra-Patel]

/gcbrun

[codecov]

Codecov Report

Merging #681 (99ff8ba) into master (2d9bfc5) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #681      +/-   ##
============================================
- Coverage     80.46%   80.45%   -0.01%     
- Complexity     1986     1987       +1     
============================================
  Files           136      136              
  Lines          8942     8944       +2     
  Branches       1030     1030              
============================================
+ Hits           7195     7196       +1     
- Misses         1333     1334       +1     
  Partials        414      414              

Flag	Coverage Δ
integrationtest	62.32% <100.00%> (-0.01%)	⬇️
unittest	74.74% <100.00%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...google/cloud/hadoop/util/RetryHttpInitializer.java	85.33% <100.00%> (+0.40%)	⬆️
...n/java/com/google/cloud/hadoop/gcsio/Watchdog.java	89.07% <0.00%> (-2.53%)	⬇️
...ogle/cloud/hadoop/gcsio/PrefixMappedItemCache.java	70.27% <0.00%> (+2.70%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[mprashanthsagar]

RetryHttpInitializer has multiple usages, Is retry on 408 safe for all of the requests which use RetryHttpInitializer ?

[medb]

I think that google-cloud-storage library added retries only for idempotent requests (GET, HEAD, etc), maybe we need to check with them and clarify if/why this additional check necessary?

cc @BenWhitehead

[BenWhitehead]

This is correct. Not all GCS operations neatly fit into the standard HTTP idempotency conventions, in some cases extra parameters need to be taken into account to determine if a request is idempotent.

These docs https://cloud.google.com/storage/docs/retry-strategy#idempotency give a good overview of some of the specifics.

[Deependra-Patel]

Good point Igor, ideally these should be handled by libraries themselves.

1. Let me know if I understand correctly. We use google-api-services-storage instead of google-cloud-storage from https://github.com/GoogleCloudDataproc/hadoop-connectors/blob/master/gcsio/pom.xml#L70.

2. Thanks for link @BenWhitehead, on same page, I can see under Client Libraries -> Java, 408 is actually retried by default. Under code snippet, I see com.google.cloud.storage.Storage as class while we use com.google.api.services.storage.Storage (code link below), is the documentation still valid?

3. @medb in the above documentation, it is mentioned that we can override default settings which we do but bit differently from what is mentioned in the sample code, we use HttpRequestInitializer instead, this may be the reason why we are not benefitting from default retry settings? https://github.com/GoogleCloudDataproc/hadoop-connectors/blob/master/gcsio/src/main/java/com/google/cloud/hadoop/gcsio/GoogleCloudStorageImpl.java#L364 cc: @BenWhitehead

[BenWhitehead]

Correct google-api-services-storage is the Discovery doc based automatically generated JSON client (aka Apiary client) (google-cloud-storage also uses this for the majority of it's RPCs) which doesn't have any automatic retry AFAIK, something like a request interceptor or request initializer is what would need to be leveraged to have the Apiary client perform multiple attempts. In google-cloud-storage we handle retries at a higher level as some of our RPCs are out of band of the apiary client.

Any links to code from google-cloud-storage are intended to be purely informational to show how that client has approached retry handling for some cases. The https://cloud.google.com/storage/docs/retry-strategy#idempotency doc is the general public which serves as a guide for anyone doing client level work against gcs (including the google-cloud-storage team as well as other customers).

[Deependra-Patel]

RetryHttpInitializer has multiple usages, Is retry on 408 safe for all of the requests which use RetryHttpInitializer ?

@mprashanthsagar According this link https://cloud.google.com/storage/docs/retry-strategy#tools, under "Client Libraries" > "Java", we can see HTTP 408 is safe to retry for all requests as already being done by latest client.

[Deependra-Patel]

/gcbrun

[medb]

HTTP 408 is safe to retry for all requests as already being done by latest client.

@Deependra-Patel I think that google-cloud-storage lib recently made retries idempotency-aware: googleapis/java-storage#1132

We should do the same - seems like it's simple enough to do based on the available request object.

Here are some examples how to check request idempotency: https://github.com/googleapis/java-storage/blob/0f5b3cb55b70944475003457278086d69e7c53e3/google-cloud-storage/src/main/java/com/google/cloud/storage/RetryAlgorithmManager.java#L238-L240