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

refactor: removes double loop of pdml transaction #361

Merged
merged 1 commit into from Jul 23, 2020
Merged

refactor: removes double loop of pdml transaction #361

merged 1 commit into from Jul 23, 2020

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.

None yet

3 participants