fix: retry pdml transaction on EOS internal error #360

thiagotnunes · 2020-07-20T23:43:14Z

The Spanner server can return an internal error closing the connection stream. When this happens an InternalException is thrown with a specific EOS message. We are seeing this specially in long running PDML tasks.

This case is now retried by the client, either by resuming the existing transaction (when there is a resume token) or by starting a new transaction.

It is possible to have the stream closed with an EOS internal error. This should be retried by the client. On this PR we add this retry logic.

thiagotnunes · 2020-07-21T01:22:46Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDMLTransaction.java

@@ -55,23 +56,6 @@
    this.rpc = rpc;
  }

-  private ByteString initTransaction() {


This method was just moved down (after the public methods).

thiagotnunes · 2020-07-21T01:23:06Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDMLTransaction.java

@@ -127,20 +111,24 @@ long executeStreamingPartitionedUpdate(final Statement statement, final Duration
                }
              }
              break;
-            } catch (UnavailableException e) {
+            } catch (UnavailableException | InternalException e) {


In this catch block we retry on EOS exception.

thiagotnunes · 2020-07-21T01:24:08Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDMLTransaction.java

+  private boolean shouldResumeOrRestartTransaction(Exception e) {
+    return e instanceof UnavailableException
+        || (e instanceof InternalException
+            && e.getMessage().contains("Received unexpected EOS on DATA frame from server"));


This is hacky, but unfortunately we do not get a specific exception for this error, so we have to proxy through the exception message. This follows the approach taken in the bigquery client: https://github.com/googleapis/java-bigquerystorage/pull/263/files.

The same error is also retried for streaming queries, and this check could probably use the existing check in SpannerExceptionFactory:

java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java

Line 233 in b264100

private static boolean isRetryable(ErrorCode code, @Nullable Throwable cause) {

. It's still just as hacky, but it would keep it confined to one method.

codecov · 2020-07-21T02:25:09Z

Codecov Report

❗ No coverage uploaded for pull request base (master@87f4f13). Click here to learn what that means.
The diff coverage is n/a.

skuruppu · 2020-07-21T04:00:55Z

@thiagotnunes I'm going to hold off on reviewing this until I better understand why this error happens. Hope it's ok to wait a couple of days until the Spanner team has had a chance to look at the issue.

skuruppu

Thanks for making this change @thiagotnunes. I would also like @olavloite to take a look before merging.

olavloite

This looks generally good to me, but could you have a look and see if it's possible to reuse the existing isRetryable method for the internal exception?

olavloite · 2020-07-22T04:58:55Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDMLTransaction.java

+  private boolean shouldResumeOrRestartTransaction(Exception e) {
+    return e instanceof UnavailableException
+        || (e instanceof InternalException
+            && e.getMessage().contains("Received unexpected EOS on DATA frame from server"));


The same error is also retried for streaming queries, and this check could probably use the existing check in SpannerExceptionFactory:

java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java

Line 233 in b264100

private static boolean isRetryable(ErrorCode code, @Nullable Throwable cause) {

. It's still just as hacky, but it would keep it confined to one method.

thiagotnunes · 2020-07-22T06:34:33Z

Thanks for the feedback @olavloite, I did not know about that functionality! I will make a change in this PR to use that.

…nto retry-pdml-on-eos

When the exception is an EOS, we should retry the exception.

Re-uses the retry logic applied by the spanner exception factory for retrying EOS internal exceptions in pdml transactions.

thiagotnunes · 2020-07-23T06:58:22Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java

@@ -257,35 +256,8 @@ private static boolean hasCauseMatching(
  }

  private static class Matchers {
-    static final Predicate<Throwable> isRetryableInternalError =


I extracted these predicates into their own classes that are now exposed to the PartitionedDmlTransaction. This was the cleanest / lowest impact way I could find to de-duplicate the EOS retryable logic.

thiagotnunes · 2020-07-24T00:21:27Z

@olavloite @skuruppu do you mind giving the PR another look? I did the following changes:

Extracted predicates from the SpannerExceptionFactory in order to use them in the PartitionedDmlTransaction, thus removing the EOS logic duplication.
Changed the PartitionedDmlTransaction class to use the extracted predicate.
Added tests for the extracted classes.

skuruppu · 2020-07-24T03:33:50Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java

+  public boolean apply(Throwable cause) {
+    if (isInternalError(cause)) {
+      if (cause.getMessage().contains(HTTP2_ERROR_MESSAGE)) {
+        // See b/25451313.


Here and below, if we could remove the references to internal issues that would be great. I don't know why they were there before. Folks working on this repo are unlikely to have access to those so no point exposing them.

No worries, will tackle this in a following PR.

olavloite · 2020-07-24T08:41:15Z

@thiagotnunes Thanks for extracting the predicates for the retryable internal errors.
I have no further comments, this looks good to me.

This is an auto-generated regeneration of the .pb.go files by cloud.google.com/go/internal/gapicgen. Once this PR is submitted, genmgr will update the corresponding CL at gocloud to depend on the newer version of go-genproto, and assign reviewers. Whilst this or any regen PR is open in go-genproto, gapicgen will not create any more regeneration PRs or CLs. If all regen PRs are closed, gapicgen will create a new set of regeneration PRs and CLs once per night. If you have been assigned to review this CL, please: - Ensure that CI is passing. If it's failing, it requires your manual attention. - Approve and submit this PR if you believe it's ready to ship. That will prompt genmgr to assign reviewers to the gocloud CL. Corresponding gocloud CL: https://code-review.googlesource.com/c/gocloud/+/56190

This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/0a285685-944f-4f44-84fb-761c5c361020/targets - [ ] To automatically regenerate this PR, check this box. Source-Link: googleapis/synthtool@b416a7b

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 20, 2020

fix: retries PDML transactions on EOS errors

8a1d690

It is possible to have the stream closed with an EOS internal error. This should be retried by the client. On this PR we add this retry logic.

thiagotnunes requested review from olavloite and skuruppu July 21, 2020 01:22

thiagotnunes commented Jul 21, 2020

View reviewed changes

thiagotnunes changed the title ~~Retry PDML Transaction on EOS Internal Error~~ fix: retry pdml transaction on EOS internal error Jul 21, 2020

thiagotnunes added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 21, 2020

skuruppu mentioned this pull request Jul 21, 2020

refactor: removes double loop of pdml transaction #361

Merged

skuruppu approved these changes Jul 21, 2020

View reviewed changes

thiagotnunes removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 21, 2020

olavloite approved these changes Jul 22, 2020

View reviewed changes

thiagotnunes added 3 commits July 23, 2020 12:00

Merge branch 'master' of https://github.com/googleapis/java-spanner i…

d3683b0

…nto retry-pdml-on-eos

fix: retries InternalException on PDML transaction

88f4bb5

When the exception is an EOS, we should retry the exception.

refactor: re-uses spanner exception retry logic

b145058

Re-uses the retry logic applied by the spanner exception factory for retrying EOS internal exceptions in pdml transactions.

thiagotnunes commented Jul 23, 2020

View reviewed changes

thiagotnunes added 4 commits July 23, 2020 17:02

fix: adds / changes headers in files

b8da6c1

fix: fixes formatting

3be1346

refactor: removes unused annotation

5edd3eb

dummy: to re-trigger tests

1ff9092

skuruppu approved these changes Jul 24, 2020

View reviewed changes

thiagotnunes merged commit a53d736 into googleapis:master Jul 26, 2020

larkee mentioned this pull request Aug 4, 2020

fix: resume iterator on EOS internal error googleapis/python-spanner#122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: retry pdml transaction on EOS internal error #360

fix: retry pdml transaction on EOS internal error #360

thiagotnunes commented Jul 20, 2020 •

edited

thiagotnunes Jul 21, 2020

thiagotnunes Jul 21, 2020

thiagotnunes Jul 21, 2020 •

edited

olavloite Jul 22, 2020

codecov bot commented Jul 21, 2020 •

edited

skuruppu commented Jul 21, 2020

skuruppu left a comment

olavloite left a comment

olavloite Jul 22, 2020

thiagotnunes commented Jul 22, 2020

thiagotnunes Jul 23, 2020

thiagotnunes commented Jul 24, 2020 •

edited

skuruppu Jul 24, 2020

thiagotnunes Jul 24, 2020

olavloite commented Jul 24, 2020

fix: retry pdml transaction on EOS internal error #360

fix: retry pdml transaction on EOS internal error #360

Conversation

thiagotnunes commented Jul 20, 2020 • edited

thiagotnunes Jul 21, 2020

Choose a reason for hiding this comment

thiagotnunes Jul 21, 2020

Choose a reason for hiding this comment

thiagotnunes Jul 21, 2020 • edited

Choose a reason for hiding this comment

olavloite Jul 22, 2020

Choose a reason for hiding this comment

codecov bot commented Jul 21, 2020 • edited

Codecov Report

skuruppu commented Jul 21, 2020

skuruppu left a comment

Choose a reason for hiding this comment

olavloite left a comment

Choose a reason for hiding this comment

olavloite Jul 22, 2020

Choose a reason for hiding this comment

thiagotnunes commented Jul 22, 2020

thiagotnunes Jul 23, 2020

Choose a reason for hiding this comment

thiagotnunes commented Jul 24, 2020 • edited

skuruppu Jul 24, 2020

Choose a reason for hiding this comment

thiagotnunes Jul 24, 2020

Choose a reason for hiding this comment

olavloite commented Jul 24, 2020

thiagotnunes commented Jul 20, 2020 •

edited

thiagotnunes Jul 21, 2020 •

edited

codecov bot commented Jul 21, 2020 •

edited

thiagotnunes commented Jul 24, 2020 •

edited