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: add cause to transaction errors on transaction commit #108

Merged
merged 2 commits into from Feb 18, 2020
Merged

fix: add cause to transaction errors on transaction commit #108

merged 2 commits into from Feb 18, 2020

Conversation

rockwotj
Copy link
Contributor

Right now if a transaction fails due to a precondition failure (which are retried, which is another bug) for example trying to update a document that doesn't exist you get a bad error message of "too many retries". Ideally retry is smarter, but this is a easy change that propagates the underlying error up to the ApiFuture result.

/cc @schmidt-sebastian @BenWhitehead

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 14, 2020
@pmakani pmakani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 14, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 14, 2020
@codecov
Copy link

codecov bot commented Feb 14, 2020

Codecov Report

Merging #108 into master will decrease coverage by 0.40%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #108      +/-   ##
============================================
- Coverage     71.97%   71.56%   -0.41%     
+ Complexity      997      968      -29     
============================================
  Files            62       62              
  Lines          5323     5202     -121     
  Branches        599      579      -20     
============================================
- Hits           3831     3723     -108     
+ Misses         1304     1299       -5     
+ Partials        188      180       -8     
Impacted Files Coverage Δ Complexity Δ
...cloud/firestore/collection/ImmutableSortedMap.java 13.95% <0.00%> (-1.96%) 1.00% <0.00%> (ø%)
...oogle/cloud/firestore/v1beta1/FirestoreClient.java 54.63% <0.00%> (-1.56%) 25.00% <0.00%> (-6.00%)
.../java/com/google/cloud/firestore/Precondition.java 68.18% <0.00%> (-1.39%) 12.00% <0.00%> (-3.00%)
...com/google/cloud/firestore/v1/FirestoreClient.java 76.28% <0.00%> (-0.86%) 31.00% <0.00%> (-6.00%)
...loud/firestore/v1beta1/stub/GrpcFirestoreStub.java 93.67% <0.00%> (-0.57%) 16.00% <0.00%> (-1.00%)
...a/com/google/cloud/firestore/FirestoreOptions.java 36.44% <0.00%> (-0.54%) 7.00% <0.00%> (ø%)
...loud/firestore/v1/stub/GrpcFirestoreAdminStub.java 94.32% <0.00%> (-0.46%) 15.00% <0.00%> (-1.00%)
...ain/java/com/google/cloud/firestore/FieldMask.java 87.09% <0.00%> (-0.41%) 13.00% <0.00%> (-2.00%)
.../firestore/v1beta1/stub/FirestoreStubSettings.java 79.26% <0.00%> (-0.38%) 22.00% <0.00%> (-1.00%)
...gle/cloud/firestore/v1/stub/GrpcFirestoreStub.java 96.04% <0.00%> (-0.36%) 19.00% <0.00%> (-1.00%)
... and 9 more

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 fcdbab3...9400caa. Read the comment docs.

@BenWhitehead
Copy link
Collaborator

@rockwotj Can you run mvn com.coveo:fmt-maven-plugin:format to get the code format pre-submit to pass?

Copy link
Collaborator

@BenWhitehead BenWhitehead left a comment

Choose a reason for hiding this comment

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

Change looks good, just need to run the formatter.

@BenWhitehead BenWhitehead added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 18, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 18, 2020
@schmidt-sebastian
Copy link
Contributor

FYI: We will shortly be addressing the transaction retry behavior across all of our SDKs.

@schmidt-sebastian
Copy link
Contributor

@rockwotj Can you update the PR title to follow conventionalcommits.org?

It this case it would be: "fix: ddd cause to transaction errors on transaction commit"

@BenWhitehead BenWhitehead changed the title Add cause to transaction errors on transaction commit fix: add cause to transaction errors on transaction commit Feb 18, 2020
@BenWhitehead BenWhitehead merged commit 00b3c6f into googleapis:master Feb 18, 2020
@BenWhitehead
Copy link
Collaborator

Thanks for the improvement @rockwotj!

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

6 participants