fix: retry PDML on Aborted and Internal errors

[olavloite]

PDML statements should be retried on the following errors:

• ABORTED: Retry using a new transaction
• INTERNAL(Received unexpected EOS on DATA frame from server):
  Retry using the same transaction and any resume token that was received.

Fixes #1197, #1177

[codecov]

Codecov Report

Merging #1205 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1205   +/-   ##
=======================================
  Coverage   98.24%   98.24%           
=======================================
  Files          23       23           
  Lines       21093    21130   +37     
  Branches     1175     1183    +8     
=======================================
+ Hits        20722    20759   +37     
  Misses        368      368           
  Partials        3        3           

Impacted Files	Coverage Δ
src/database.ts	99.92% <100.00%> (+<0.01%)	⬆️
src/partial-result-stream.ts	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 445015f...0bcc3ad. Read the comment docs.

[thiagotnunes]

LGTM, but we will need to internally test it to make sure. I will try and do that next week. Thanks!

[skuruppu]

nit: remove commented out line.

[skuruppu]

Adding a "do not merge" until @thiagotnunes tests the implementation.

[thiagotnunes]

@olavloite Still getting an error from the backend (a different one though): ERROR: Error: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error)

If we change the isRetryable function to something like this it should be enough:

function isRetryableInternalError(err: grpc.ServiceError): boolean {
  return (
    err.code === grpc.status.INTERNAL &&
    (err.message.includes('Received unexpected EOS on DATA frame from server') || err.message.includes('Received RST_STREAM'))
  );
}

[olavloite]

@thiagotnunes Thanks a lot for testing it. I've updated the check to include this error as well.

I also had a quick look at a couple of the other client libraries:

1. In Go we also retry this error: https://github.com/googleapis/google-cloud-go/blob/14d43f9dd504466f50d8764d7dd346dce29ed98e/spanner/retry.go#L63
2. In Java we do not: https://github.com/googleapis/java-spanner/blob/6f47b8a759643f772230df0c2e153338d44f70ce/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java#L35

Could it be that this is not necessary in Java, because we set a keep-alive on the gRPC transport channel in Java: https://github.com/googleapis/java-spanner/blob/6f47b8a759643f772230df0c2e153338d44f70ce/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java#L316

[thiagotnunes]

@olavloite thanks for the fix!

I don't know why this is not happening in Java, but you might have a good lead there. By searching internally, it seems that for other clients have decided to also add this error as retryable in any case. I think we should add that in the java client to be on the safe side.