Skip to content

Conversation

@thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Jul 21, 2020

Refactors the PDML transaction class to have a single loop for resumes / retries.
I tried to simplify as much as possible on the retry logic we were doing on the transaction.

<rant>
Unfortunately with Java 7 it is hard to improve this logic by not using exceptions for that without having to write lots of code. I could not also come up with a good immutable approach that would have better readability.
</rant>

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 21, 2020
@thiagotnunes thiagotnunes added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 21, 2020
@thiagotnunes
Copy link
Contributor Author

Since this contains a fix for retrying InternalException - EOS, we will wait until the spanner team has reviewed the code before merging.
@skuruppu I could also remove this fix, so that we can merge this right away.

@skuruppu
Copy link
Contributor

Do you mean that this also contains the change from #360? If that's the case, yeah it would be ideal if we decouple the two fixes.

@thiagotnunes
Copy link
Contributor Author

@skuruppu will remove the fix from here

Refactors the PDML transaction class to have a single loop for resumes /
retries.
@thiagotnunes
Copy link
Contributor Author

@skuruppu the fix is removed from this PR

Copy link
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

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

LGTM, but I"ll leave it to @skuruppu whether we would want to merge this before the fix for retrying the specific internal errors or not.

@@ -0,0 +1,196 @@
/*
* Copyright 2019 Google LLC
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 2020

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix on the next PR (#360).

@thiagotnunes thiagotnunes removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 23, 2020
@thiagotnunes thiagotnunes merged commit 87f4f13 into googleapis:master Jul 23, 2020
@thiagotnunes thiagotnunes deleted the pdml-transaction-refactoring branch July 23, 2020 01:43
ansh0l pushed a commit to ansh0l/java-spanner that referenced this pull request Nov 10, 2022
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/+/56410
rajatbhatta pushed a commit to rajatbhatta/java-spanner that referenced this pull request Nov 17, 2022
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/8440e2da-528d-412a-bf89-385b5618d6dd/targets

- [ ] To automatically regenerate this PR, check this box.

Source-Link: googleapis/synthtool@1aeca92
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants