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

feat: base transaction retries on error codes #129

Merged

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Mar 11, 2020

Port of googleapis/nodejs-firestore#953

Implements go/transaction-retry-matrix for Java and cleans up some of the tests:

  • Transaction IDs are passed as string to the helpers
  • Request/response validation is centralized

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 11, 2020
@schmidt-sebastian schmidt-sebastian changed the title feat: base transaction on error codes feat: base transaction retries on error codes Mar 11, 2020
@schmidt-sebastian schmidt-sebastian force-pushed the mrschmidt/transactionretries branch 2 times, most recently from 79a8dd2 to 0aafdb5 Compare March 12, 2020 00:04
@codecov
Copy link

codecov bot commented Mar 26, 2020

Codecov Report

Merging #129 into master will increase coverage by 0.96%.
The diff coverage is 98.26%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #129      +/-   ##
============================================
+ Coverage     71.62%   72.58%   +0.96%     
- Complexity      969      988      +19     
============================================
  Files            62       63       +1     
  Lines          5222     5270      +48     
  Branches        579      590      +11     
============================================
+ Hits           3740     3825      +85     
+ Misses         1302     1259      -43     
- Partials        180      186       +6
Impacted Files Coverage Δ Complexity Δ
...n/java/com/google/cloud/firestore/Transaction.java 97.5% <100%> (-0.28%) 10 <0> (-1)
.../com/google/cloud/firestore/TransactionRunner.java 100% <100%> (ø) 4 <4> (?)
...java/com/google/cloud/firestore/FirestoreImpl.java 78.62% <100%> (-3.73%) 24 <1> (ø)
...rc/main/java/com/google/cloud/firestore/Watch.java 91.55% <100%> (-0.26%) 56 <0> (-1)
...com/google/cloud/firestore/TransactionOptions.java 75% <80%> (+3.57%) 6 <3> (ø) ⬇️
...com/google/cloud/firestore/FirestoreException.java 83.33% <83.33%> (+3.33%) 9 <3> (+1) ⬆️
...rc/main/java/com/google/cloud/firestore/Query.java 90.99% <0%> (-0.74%) 100% <0%> (+6%)
...gle/cloud/firestore/v1beta1/FirestoreSettings.java 13.2% <0%> (ø) 2% <0%> (ø) ⬇️
... and 5 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 8ca0ea8...3cefd5f. Read the comment docs.

@schmidt-sebastian
Copy link
Contributor Author

@BenWhitehead Can you ask someone to review or review this yourself if you have time? This one is a little trickier :/

@BenWhitehead BenWhitehead self-assigned this Mar 26, 2020
@BenWhitehead
Copy link
Collaborator

@schmidt-sebastian Sure, I'll dig into this tomorrow first thing.

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.

Mostly looks okay. A few questions and one request to move a method.

Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

Thank you for the review! All feedback addressed.

@schmidt-sebastian schmidt-sebastian added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 27, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 27, 2020
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.

LGTM I'm looking into the Linkage Checker status check failure and will let you know if it requires anything to be change in the PR.

@BenWhitehead BenWhitehead merged commit 00b6eb3 into googleapis:master Apr 1, 2020
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

4 participants