KEMBAR78
fix: add cause to transaction errors on transaction commit by rockwotj · Pull Request #108 · googleapis/java-firestore · GitHub
Skip to content

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.

6 participants