Skip to content

fix: retry transactions that fail with expired transaction IDs#447

Merged
schmidt-sebastian merged 2 commits intomasterfrom
mrschmidt/illegalargument
Nov 5, 2020
Merged

fix: retry transactions that fail with expired transaction IDs#447
schmidt-sebastian merged 2 commits intomasterfrom
mrschmidt/illegalargument

Conversation

@schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Nov 2, 2020

Port of googleapis/nodejs-firestore#1347

Once a transaction expires, the backend returns INVALID_ARGUMENT. While INVALID_ARGUMENT is not generally retryable, we want to retry INVALID_ARGUMENT for expired transactions only. We should only retry if the error message contains "transaction has expired" and if we have previously received a transaction ID.

This PR aligns the Java SDK a little more with the Node SDK:

  • Rollback RPC is the first step in transaction processing, which allows it to use the same ApiFutures.catchingAsync for all error handling.

There are some test updates:

  • I had to drop the Map from the ResponseStubber since one of the tests I created has two Rollback RPCs, which ended up overwriting each other in the Map implementation (based on key equality). Instead, I am using an ArrayList.
  • I centralized some of the request validation in ResponseStubber.

@schmidt-sebastian schmidt-sebastian requested a review from a team as a code owner November 2, 2020 23:13
@schmidt-sebastian schmidt-sebastian requested a review from a team November 2, 2020 23:13
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 2, 2020
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label Nov 2, 2020
yoshi-code-bot
yoshi-code-bot reviewed Nov 2, 2020
View reviewed changes
Copy link

@yoshi-code-bot yoshi-code-bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java
    • lines 295-295

@schmidt-sebastian schmidt-sebastian force-pushed the mrschmidt/illegalargument branch from 24ca8d8 to 94c513b Compare November 2, 2020 23:16
@codecov
Copy link

codecov bot commented Nov 2, 2020 •
edited
Loading

Codecov Report

Merging #447 into master will decrease coverage by 0.01%.
The diff coverage is 97.14%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #447      +/-   ##
============================================
- Coverage     73.39%   73.38%   -0.02%     
- Complexity     1090     1094       +4     
============================================
  Files            65       65              
  Lines          5785     5786       +1     
  Branches        721      721              
============================================
  Hits           4246     4246              
  Misses         1313     1313              
- Partials        226      227       +1
Impacted Files Coverage Δ Complexity Δ
.../com/google/cloud/firestore/TransactionRunner.java 99.03% <96.77%> (-0.97%) 7.00 <2.00> (+2.00) ⬇️
...n/java/com/google/cloud/firestore/Transaction.java 85.41% <100.00%> (-0.86%) 13.00 <2.00> (+2.00) ⬇️

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 e241f8e...a604178. Read the comment docs.

yoshi-code-bot
yoshi-code-bot reviewed Nov 2, 2020
View reviewed changes
Copy link

@yoshi-code-bot yoshi-code-bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • google-cloud-firestore/src/main/java/com/google/cloud/firestore/TransactionRunner.java
    • lines 295-295

@schmidt-sebastian schmidt-sebastian added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 3, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 3, 2020
@schmidt-sebastian schmidt-sebastian merged commit 5905438 into master Nov 5, 2020
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/illegalargument branch November 5, 2020 22:00
gcf-merge-on-green bot pushed a commit that referenced this pull request Jan 20, 2021
@release-please
chore: release 2.2.0 (#369)
f6ffc22 
🤖 I have created a release \*beep\* \*boop\* 
---
## [2.2.0](https://www.github.com/googleapis/java-firestore/compare/v2.1.0...v2.2.0) (2021-01-20)


### Features

* Add bundle proto building ([#271](https://www.github.com/googleapis/java-firestore/issues/271)) ([994835c](https://www.github.com/googleapis/java-firestore/commit/994835c0a3be077404afa60abd4d4685d17fb533))
* add bundle.proto from googleapis/googleapis ([#407](https://www.github.com/googleapis/java-firestore/issues/407)) ([37da386](https://www.github.com/googleapis/java-firestore/commit/37da386875d1b65121e8a9a92b1a000537f07625))
* add CollectionGroup#getPartitions(long) ([#478](https://www.github.com/googleapis/java-firestore/issues/478)) ([bab064e](https://www.github.com/googleapis/java-firestore/commit/bab064edde26325bf0041ffe28d4c63b97a089c5))
* add implicit ordering for startAt(DocumentReference) calls ([#417](https://www.github.com/googleapis/java-firestore/issues/417)) ([aae6dc9](https://www.github.com/googleapis/java-firestore/commit/aae6dc960f7c42830ceed23c65acaad3e457dcff))
* add max/min throttling options to BulkWriterOptions ([#400](https://www.github.com/googleapis/java-firestore/issues/400)) ([27a9397](https://www.github.com/googleapis/java-firestore/commit/27a9397f67e151d723241c80ccb2ec9f1bfbba1c))
* add success and error callbacks to BulkWriter ([#483](https://www.github.com/googleapis/java-firestore/issues/483)) ([3c05037](https://www.github.com/googleapis/java-firestore/commit/3c05037e8fce8d3ce4907fde85bd254fc98ea588))
* Implementation of Firestore Bundle Builder ([#293](https://www.github.com/googleapis/java-firestore/issues/293)) ([fd5ef90](https://www.github.com/googleapis/java-firestore/commit/fd5ef90b6681cc67aeee6c95f3de80267798dcd0))
* Release bundles ([#466](https://www.github.com/googleapis/java-firestore/issues/466)) ([3af065e](https://www.github.com/googleapis/java-firestore/commit/3af065e32b193931c805b576f410ad90124b43a7))


### Bug Fixes

* add @BetaApi, make BulkWriter public, and refactor Executor ([#497](https://www.github.com/googleapis/java-firestore/issues/497)) ([27ff9f6](https://www.github.com/googleapis/java-firestore/commit/27ff9f6dfa92cac9119d2014c24a0759baa44fb7))
* **build:** sample checkstyle violations ([#457](https://www.github.com/googleapis/java-firestore/issues/457)) ([777ecab](https://www.github.com/googleapis/java-firestore/commit/777ecabd1ce12cbc5f4169de6c23a90f98deac06))
* bulkWriter: writing to the same doc doesn't create a new batch ([#394](https://www.github.com/googleapis/java-firestore/issues/394)) ([259ece8](https://www.github.com/googleapis/java-firestore/commit/259ece8511db71ea79cc1a080eb785a15db88756))
* empty commit to trigger release-please ([fcef0d3](https://www.github.com/googleapis/java-firestore/commit/fcef0d302cd0a9339d82db73152289d6f9f67ff2))
* make BulkWriterOptions public ([#502](https://www.github.com/googleapis/java-firestore/issues/502)) ([6ea05be](https://www.github.com/googleapis/java-firestore/commit/6ea05beb3f27337bef910ca64f0e3f32de6b7e98))
* retry Query streams ([#426](https://www.github.com/googleapis/java-firestore/issues/426)) ([3513cd3](https://www.github.com/googleapis/java-firestore/commit/3513cd39ff43d26c8432c05ce20693350539ae8f))
* retry transactions that fail with expired transaction IDs ([#447](https://www.github.com/googleapis/java-firestore/issues/447)) ([5905438](https://www.github.com/googleapis/java-firestore/commit/5905438af6501353e978210808834a26947aae95))
* verify partition count before invoking GetPartition RPC ([#418](https://www.github.com/googleapis/java-firestore/issues/418)) ([2054ae9](https://www.github.com/googleapis/java-firestore/commit/2054ae971083277e1cf81c2b57500c40a6faa0ef))


### Documentation

* **sample:** normalize firestore sample's region tags ([#453](https://www.github.com/googleapis/java-firestore/issues/453)) ([b529245](https://www.github.com/googleapis/java-firestore/commit/b529245c75f770e1b47ca5d9561bab55a7610677))


### Dependencies

* remove explicit version for jackson ([#479](https://www.github.com/googleapis/java-firestore/issues/479)) ([e2aecfe](https://www.github.com/googleapis/java-firestore/commit/e2aecfec51465b8fb3413337a76f9a3de57b8500))
* update dependency com.google.cloud:google-cloud-conformance-tests to v0.0.12 ([#367](https://www.github.com/googleapis/java-firestore/issues/367)) ([2bdd846](https://www.github.com/googleapis/java-firestore/commit/2bdd84693bbd968cafabd0e7ee56d1a9a7dc31ca))
* update dependency com.google.cloud:google-cloud-conformance-tests to v0.0.13 ([#411](https://www.github.com/googleapis/java-firestore/issues/411)) ([e6157b5](https://www.github.com/googleapis/java-firestore/commit/e6157b5cb532e0184125355b12115058e72afa67))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.10.0 ([#383](https://www.github.com/googleapis/java-firestore/issues/383)) ([cb39ee8](https://www.github.com/googleapis/java-firestore/commit/cb39ee820c2f67e22da623f5a6eaa7ee6bf351e2))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.10.2 ([#403](https://www.github.com/googleapis/java-firestore/issues/403)) ([991dd81](https://www.github.com/googleapis/java-firestore/commit/991dd810360e654fca0b53e0611da0cd77febc7c))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.12.1 ([#425](https://www.github.com/googleapis/java-firestore/issues/425)) ([b897ffa](https://www.github.com/googleapis/java-firestore/commit/b897ffa90427a8f597c02c24f80d1d162be48b23))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.13.0 ([#430](https://www.github.com/googleapis/java-firestore/issues/430)) ([0f8f218](https://www.github.com/googleapis/java-firestore/commit/0f8f218678c3ddebb73748c382cab8e38c2f140d))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.14.1 ([#446](https://www.github.com/googleapis/java-firestore/issues/446)) ([e241f8e](https://www.github.com/googleapis/java-firestore/commit/e241f8ebbfdf202f00424177c69962311b37fc88))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.15.0 ([#460](https://www.github.com/googleapis/java-firestore/issues/460)) ([b82fc35](https://www.github.com/googleapis/java-firestore/commit/b82fc3561d1a397438829ab69df24141481369a2))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.16.0 ([#481](https://www.github.com/googleapis/java-firestore/issues/481)) ([ae98824](https://www.github.com/googleapis/java-firestore/commit/ae988245e6d6391c85414e9b6f7ae1b8148c3a6d))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.16.1 ([4ace93c](https://www.github.com/googleapis/java-firestore/commit/4ace93c7be580a8f7870e71cad2dc19bb5fdef29))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.17.0 ([#487](https://www.github.com/googleapis/java-firestore/issues/487)) ([e11e472](https://www.github.com/googleapis/java-firestore/commit/e11e4723bc75727086bee0436492f458def29ff5))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.18.0 ([#495](https://www.github.com/googleapis/java-firestore/issues/495)) ([f78720a](https://www.github.com/googleapis/java-firestore/commit/f78720a155f1294321f05266b9a546bbf2cb9a04))
* update jackson dependencies to v2.11.3 ([#396](https://www.github.com/googleapis/java-firestore/issues/396)) ([2e176e2](https://www.github.com/googleapis/java-firestore/commit/2e176e2f864262f31e6f93705fa7e794023b9649))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BenWhitehead BenWhitehead BenWhitehead approved these changes

@wu-hui wu-hui wu-hui approved these changes

+1 more reviewer

@yoshi-code-bot yoshi-code-bot yoshi-code-bot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@BenWhitehead BenWhitehead

Labels

api: firestore Issues related to the googleapis/java-firestore API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@schmidt-sebastian @BenWhitehead @wu-hui @yoshi-code-bot @yoshi-kokoro