fix: retry PDML on Aborted and Internal errors#1205
fix: retry PDML on Aborted and Internal errors#1205thiagotnunes merged 4 commits intogoogleapis:masterfrom
Conversation
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 googleapis#1197
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
thiagotnunes
left a comment
There was a problem hiding this comment.
LGTM, but we will need to internally test it to make sure. I will try and do that next week. Thanks!
test/database.ts
Outdated
| ) as sinon.SinonStub).callsFake(callback => callback(null)); | ||
|
|
||
| runUpdateStub = sandbox.stub(fakePartitionedDml, 'runUpdate'); | ||
| // runUpdateStub = sandbox.stub(fakePartitionedDml, 'runUpdate'); |
There was a problem hiding this comment.
nit: remove commented out line.
|
Adding a "do not merge" until @thiagotnunes tests the implementation. |
|
@olavloite Still getting an error from the backend (a different one though): If we change the 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'))
);
} |
|
@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:
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 |
|
@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. |
PDML statements should be retried on the following errors:
Retry using the same transaction and any resume token that was received.
Fixes #1197, #1177