Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: PDML retry settings were not applied for aborted tx #232

Merged
merged 2 commits into from May 20, 2020

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented May 20, 2020

The PartitionedDML retry settings were only applied for the RPC, and not for the generic retryer that would retry the PDML transaction if it was aborted by Spanner. This could cause long-running PDML transactions to fail with an Aborted exception.

Fixes #199

The PartitionedDML retry settings were only applied for the RPC, and not
for the generic retryer that would retry the PDML transaction if it was
aborted by Spanner. This could cause long-running PDML transactions to
fail with an Aborted exception.

Fixes #199
@googlebot googlebot added the cla: yes label May 20, 2020
@skuruppu skuruppu self-requested a review May 20, 2020
@skuruppu skuruppu merged commit 308a465 into master May 20, 2020
13 checks passed
@skuruppu skuruppu deleted the use-pdml-retry-settings-on-retry-after-abort branch May 20, 2020
@@ -300,7 +302,7 @@ public GapicSpannerRpc(final SpannerOptions options) {

// Set a keepalive time of 120 seconds to help long running
// commit GRPC calls succeed
.setKeepAliveTime(Duration.ofSeconds(GRPC_KEEPALIVE_SECONDS))
.setKeepAliveTime(Duration.ofSeconds(GRPC_KEEPALIVE_SECONDS * 1000))
Copy link

@ejona86 ejona86 Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you really intend to set the keepalive time to be over a day? This is a value of 120,000 seconds.

Copy link
Contributor Author

@olavloite olavloite Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Thanks for noticing. Nope, that's a typical seconds/milliseconds mistake.

Copy link
Contributor Author

@olavloite olavloite Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants