More aggressive retry strategy for NIO by jean-philippe-martin · Pull Request #2083 · googleapis/google-cloud-java

jean-philippe-martin · 2017-05-22T20:12:28Z

(but only when OptionMaxChannelReopens is set)

Testing shows that sometimes storage will fail with one of the errors below when load is high. In these situations, the ChannelReopen policy is to attempt to re-open the channel after a delay.

(but only when OptionMaxChannelReopens is set)

coveralls · 2017-05-22T20:23:49Z

Coverage increased (+0.01%) to 80.947% when pulling b1408f0 on jean-philippe-martin:jp_aggressive_reopen into 0fbd82e on GoogleCloudPlatform:master.

Sign in to view

-            // this error isn't marked as retryable since the channel is closed;
+          if (reopens < maxChannelReopens && (
+              exs.getMessage().contains("Connection closed prematurely")
+          || exs.getCause() instanceof javax.net.ssl.SSLHandshakeException


Sign in to view

+              exs.getMessage().contains("Connection closed prematurely")
+          || exs.getCause() instanceof javax.net.ssl.SSLHandshakeException
+          || exs.getCause() instanceof java.io.EOFException
+          || exs.getCause() instanceof java.net.SocketTimeoutException


Sign in to view

+          || exs.getCause() instanceof javax.net.ssl.SSLHandshakeException
+          || exs.getCause() instanceof java.io.EOFException
+          || exs.getCause() instanceof java.net.SocketTimeoutException
+          )) {


Sign in to view

-          if (exs.getMessage().contains("Connection closed prematurely") && reopens < maxChannelReopens) {
-            // this error isn't marked as retryable since the channel is closed;
+          if (reopens < maxChannelReopens && (
+              exs.getMessage().contains("Connection closed prematurely")


Sign in to view

            continue;
          }
-          throw exs;
+          throw new StorageException(exs.getCode(), "Retry failed", exs);


Sign in to view


  private void sleepForAttempt(int attempt) {
+    long delay = 500;
+    // exponential backoff


droazen · 2017-05-25T21:44:27Z

Added my feedback @jean-philippe-martin. Thanks for doing this, it will (hopefully) solve a major issue for us with our use of the gcloud NIO functionality!

- longer timeout - check exs.getCause() as well - retry for more exception classes - better phrasing

Sign in to view

+    return exs != null && (exs.getMessage().contains("Connection closed prematurely")
+            || exs.getCause() instanceof javax.net.ssl.SSLException
+            || exs.getCause() instanceof java.io.EOFException
+            || exs.getCause() instanceof java.net.SocketException);


Sign in to view

-          if (exs.getMessage().contains("Connection closed prematurely") && reopens < maxChannelReopens) {
-            // this error isn't marked as retryable since the channel is closed;
-            // but here at this higher level we can retry it.
+          if (reopens < maxChannelReopens && (isReopenable(exs) || isReopenable(exs.getCause()))) {


coveralls · 2017-05-25T22:09:18Z

Coverage decreased (-0.03%) to 80.903% when pulling b2bb415 on jean-philippe-martin:jp_aggressive_reopen into 0fbd82e on GoogleCloudPlatform:master.

Also added a test for getCause().getCause()

coveralls · 2017-05-25T23:37:13Z

Coverage decreased (-0.03%) to 80.902% when pulling 2fddc77 on jean-philippe-martin:jp_aggressive_reopen into 0fbd82e on GoogleCloudPlatform:master.

coveralls · 2017-05-26T00:10:10Z

Coverage decreased (-0.03%) to 80.903% when pulling 7c81e40 on jean-philippe-martin:jp_aggressive_reopen into 0fbd82e on GoogleCloudPlatform:master.

droazen · 2017-05-26T16:38:21Z

👍 Latest version looks good to me -- hope we can get this merged soon!

Sign in to view

+    Throwable throwable = exs;
+    while (throwable != null) {
+      if (throwable.getMessage().contains("Connection closed prematurely")
+          || throwable.getCause() instanceof javax.net.ssl.SSLException


Sign in to view

      int amt;
      int retries = 0;
-      int maxRetries = 3;
+      int maxRetries = Math.max(3, maxChannelReopens);


Sign in to view

+    long maxDelay = 120_000;
+    // exponential backoff
+    for (int i=0; i<attempt; i++) {
+      delay *= 2;


Sign in to view

+    long delay = 1000;
+    long maxDelay = 120_000;
+    // exponential backoff
+    for (int i=0; i<attempt; i++) {


Sign in to view

+    long maxDelay = 120_000;
+    // exponential backoff
+    for (int i=0; i<attempt; i++) {
+      delay *= 2;


Simplify backoff computation, add loop variant variable, and code formatting changes.

Sign in to view

+    long firstdelay = 1000;
+    long maxDelay = 120_000;
+    // exponential backoff
+    long delay = firstdelay * Math.min(1 << attempt, maxDelay);


coveralls · 2017-05-26T18:16:08Z

Coverage decreased (-0.03%) to 80.903% when pulling 1593376 on jean-philippe-martin:jp_aggressive_reopen into 0fbd82e on GoogleCloudPlatform:master.

coveralls · 2017-05-26T21:20:23Z

Coverage decreased (-0.02%) to 80.914% when pulling cc035c9 on jean-philippe-martin:jp_aggressive_reopen into 0fbd82e on GoogleCloudPlatform:master.

vam-google

LGTM, but please address the last comment about if statement first.

Sign in to view

+    long delay = 1000L * (1L << Math.min(attempt, 7));
    try {
-      Thread.sleep((attempt - 1) * 500);
+      Thread.sleep(delay);


Sign in to view

+    int maxDepth = 10;
+    while (throwable != null && maxDepth-- > 0) {
+      Throwable cause = throwable.getCause();
+      if (throwable.getMessage().contains("Connection closed prematurely")


Also simplify 'isReopenable' code, make its test harder, and add a comment about why 2x backoff.

jean-philippe-martin · 2017-05-30T17:35:17Z

@vam-google I'm not sure which comment you mean. What I see is the last comment on the if statement is the concern about the loop not terminating but I have addressed that with maxDepth. I apologize, but could you please point me to the part that still needs a fix?

vam-google

One last comment

Sign in to view

+    // ensures finite iteration
+    int maxDepth = 10;
+    while (throwable != null && maxDepth-- > 0) {
+      if (throwable.getMessage().contains("Connection closed prematurely")


vam-google · 2017-05-30T17:40:59Z

@jean-philippe-martin sorry, I meant the comment about extracting check from the loop. But I was wrong about it, wo it is ok. But I added one more comment. Please address it, and then LGTM.

vam-google

LGTM

Sign in to view

+          } else if (exs.isRetryable() || exs.getCode() == 500 || exs.getCode() == 503) {
            retries++;
+            if (retries > maxRetries) {
+              throw new StorageException(exs.getCode(), "All retries failed", exs);


vam-google · 2017-05-30T18:18:58Z

Ok, I'm going to merge this pull request. @jean-philippe-martin please confirm that you are done with your changes.

(comment explains how you may get a retriable exception that says "retries failed")

jean-philippe-martin · 2017-05-30T18:22:10Z

Now that I added the comment, I'm done! Thanks for the review and for doing the merge (since I cannot). I suppose we want to wait for the Travis test to pass, though, yes?

coveralls · 2017-05-30T18:41:19Z

Coverage increased (+0.05%) to 80.981% when pulling 4161392 on jean-philippe-martin:jp_aggressive_reopen into 0fbd82e on GoogleCloudPlatform:master.

coveralls · 2017-05-30T18:57:07Z

Coverage increased (+0.05%) to 80.981% when pulling aba972d on jean-philippe-martin:jp_aggressive_reopen into 0fbd82e on GoogleCloudPlatform:master.

coveralls · 2017-05-30T19:07:41Z

Coverage increased (+0.05%) to 80.981% when pulling 4161392 on jean-philippe-martin:jp_aggressive_reopen into 0fbd82e on GoogleCloudPlatform:master.

jean-philippe-martin · 2017-05-30T19:23:27Z

Great, Travis is green! @vam-google I leave it to you to merge in the code. Thank you very much!

vam-google · 2017-05-31T01:10:25Z

Merged. Thanks @jean-philippe-martin for your work!

…googleapis#2083)

@jean-philippe-martin

by @jean-philippe-martin

@jean-philippe-martin

by @jean-philippe-martin

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

#2083)

More aggressive retry strategy for NIO

b1408f0

(but only when OptionMaxChannelReopens is set)

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 22, 2017

jean-philippe-martin mentioned this pull request May 22, 2017

NIO SSL Exception in GenomicsDBImport broadinstitute/gatk#2685

Closed

droazen reviewed May 25, 2017

View reviewed changes

Improve as per review comments

b2bb415

- longer timeout - check exs.getCause() as well - retry for more exception classes - better phrasing

droazen reviewed May 25, 2017

View reviewed changes

Review feedback

2fddc77

Also added a test for getCause().getCause()

Fix reviewer and machine comments

7c81e40

vam-google reviewed May 26, 2017

View reviewed changes

Address reviewer comments

1593376

Simplify backoff computation, add loop variant variable, and code formatting changes.

vam-google reviewed May 26, 2017

View reviewed changes

Fix bad delay code

cc035c9

vam-google approved these changes May 30, 2017

View reviewed changes

'retry failed' message only shows when there was a retry

83eee2e

Also simplify 'isReopenable' code, make its test harder, and add a comment about why 2x backoff.

vam-google reviewed May 30, 2017

View reviewed changes

Add missing null check

aba972d

vam-google approved these changes May 30, 2017

View reviewed changes

Add comment to "All retries failed" exception

4161392

(comment explains how you may get a retriable exception that says "retries failed")

vam-google merged commit 6aa8964 into googleapis:master May 31, 2017

jean-philippe-martin deleted the jp_aggressive_reopen branch May 31, 2017 16:39

This was referenced Jun 2, 2017

Date for next release? #2110

Closed

GCS 503s should be retried broadinstitute/gatk#2749

Closed

suztomo pushed a commit to suztomo/google-cloud-java that referenced this pull request Mar 23, 2026

deps: update dependency io.opentelemetry:opentelemetry-bom to v1.51.0 (…

2a9af6a

…googleapis#2083)

suztomo pushed a commit that referenced this pull request Mar 30, 2026

More aggressive retry strategy for NIO (#2083)

1c6ed54

by @jean-philippe-martin

lqiu96 pushed a commit that referenced this pull request Apr 1, 2026

More aggressive retry strategy for NIO (#2083)

4ad8429

by @jean-philippe-martin

meltsufin pushed a commit that referenced this pull request Apr 29, 2026

chore(main): release 1.131.0 (#2083)

0cb6a57

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

meltsufin pushed a commit that referenced this pull request May 1, 2026

chore(main): release 1.131.0 (#2083)

a1c4676

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

meltsufin pushed a commit that referenced this pull request May 2, 2026

chore: Update generation configuration at Wed Apr 23 16:48:26 UTC 2025 (

6ab0136

#2083)

Conversation

jean-philippe-martin commented May 22, 2017

Uh oh!

coveralls commented May 22, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

droazen commented May 25, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

coveralls commented May 25, 2017

Uh oh!

coveralls commented May 25, 2017

Uh oh!

coveralls commented May 26, 2017

Uh oh!

droazen commented May 26, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

vam-google commented May 30, 2017 •

edited

Loading