Skip to content

fix: Make rollback best effort.#1515

Merged
tom-andersen merged 3 commits intomainfrom
tomandersen/b/318686162
Jan 18, 2024
Merged

fix: Make rollback best effort.#1515
tom-andersen merged 3 commits intomainfrom
tomandersen/b/318686162

Conversation

@tom-andersen
Copy link
Copy Markdown
Contributor

@tom-andersen tom-andersen commented Jan 4, 2024 •
edited
Loading

Internal tracking: b/318686162

A rollback is meant to be best effort. If the transaction has already expired, it is possible for the rollback to fail due to transaction no longer existing in Firestore. The retry logic will use attempts to rollback, and in the case where transaction no longer exists, all attempts to be exhausted attempted to rollback transaction.

This PR makes the rollback best effort, simply logging any rollback error and continuing.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: firestore Issues related to the googleapis/java-firestore API. labels Jan 4, 2024
@tom-andersen tom-andersen marked this pull request as ready for review January 4, 2024 19:40
@tom-andersen tom-andersen requested a review from a team as a code owner January 4, 2024 19:40
@tom-andersen tom-andersen requested a review from ehsannas January 4, 2024 19:41
@tom-andersen tom-andersen added the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 4, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 4, 2024
@tom-andersen tom-andersen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 4, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 4, 2024
ehsannas
ehsannas reviewed Jan 4, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

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

Even though the documentation suggests that rollbacks are best-effort, we may want to still keep up the effort to retry the rollback unless we're positive the retry will fail (i.e. for INTERNAL error code).

google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java Outdated
Comment on lines +1200 to +1201
System.out.println("BAD INVOCATION");
System.out.println(invocationOnMock.getArguments()[0]);
Copy link
Copy Markdown
Contributor

@ehsannas ehsannas Jan 4, 2024

Choose a reason for hiding this comment

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

remove system.out

Copy link
Copy Markdown
Contributor Author

@tom-andersen tom-andersen Jan 5, 2024

Choose a reason for hiding this comment

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

Removed

@ehsannas ehsannas assigned tom-andersen and unassigned ehsannas Jan 5, 2024
@tom-andersen tom-andersen force-pushed the tomandersen/b/318686162 branch from 4d0835c to 4e94636 Compare January 5, 2024 20:29
@tom-andersen tom-andersen added the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 5, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 5, 2024
@tom-andersen
Copy link
Copy Markdown
Contributor Author

tom-andersen commented Jan 5, 2024

Even though the documentation suggests that rollbacks are best-effort, we may want to still keep up the effort to retry the rollback unless we're positive the retry will fail (i.e. for INTERNAL error code).

I verified that GAPIC does do the retries from rollback appropriately following the service config. So we can safely remove this extra layer that was simply retying errors that couldn't be retried anyway.

@tom-andersen tom-andersen requested a review from ehsannas January 5, 2024 20:33
@tom-andersen tom-andersen added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jan 5, 2024
@tom-andersen tom-andersen removed their assignment Jan 11, 2024
@ehsannas ehsannas assigned tom-andersen and unassigned ehsannas Jan 17, 2024
@tom-andersen tom-andersen merged commit 4c39af5 into main Jan 18, 2024
@tom-andersen tom-andersen deleted the tomandersen/b/318686162 branch January 18, 2024 15:43
@release-please release-please bot mentioned this pull request Jan 18, 2024
Merged
gcf-merge-on-green bot pushed a commit that referenced this pull request Jan 25, 2024
@release-please
chore(main): release 3.16.1 (#1528)
de842a0 
🤖 I have created a release *beep* *boop*
---


## [3.16.1](https://togithub.com/googleapis/java-firestore/compare/v3.16.0...v3.16.1) (2024-01-25)


### Bug Fixes

* `internalStream` should handle duplicate `onComplete`s. ([#1523](https://togithub.com/googleapis/java-firestore/issues/1523)) ([b3067d7](https://togithub.com/googleapis/java-firestore/commit/b3067d7b382ea5c4c9124a12a701abe2f7289503))
* Allow an explicit MustExist precondition for update ([#1542](https://togithub.com/googleapis/java-firestore/issues/1542)) ([46e09aa](https://togithub.com/googleapis/java-firestore/commit/46e09aad7f6689d4dfe82bd284905d52edda4364))
* **deps:** Update the Java code generator (gapic-generator-java) to 2.32.0 ([#1534](https://togithub.com/googleapis/java-firestore/issues/1534)) ([2281320](https://togithub.com/googleapis/java-firestore/commit/2281320fd924a1d6c259a493ce703a51f0cd8a03))
* Make rollback best effort. ([#1515](https://togithub.com/googleapis/java-firestore/issues/1515)) ([4c39af5](https://togithub.com/googleapis/java-firestore/commit/4c39af50d6d416440164fc5d5360f3912cd8f01b))
* Thread safe UpdateBuilder ([#1537](https://togithub.com/googleapis/java-firestore/issues/1537)) ([f9cdab5](https://togithub.com/googleapis/java-firestore/commit/f9cdab5885bd1d500c6fc412eb3090cea9347d0e))


### Dependencies

* Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.22.0 ([#1535](https://togithub.com/googleapis/java-firestore/issues/1535)) ([04c0e07](https://togithub.com/googleapis/java-firestore/commit/04c0e0736ddcd49eb42aacb31e2fc087b2a39754))
* Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.23.0 ([#1544](https://togithub.com/googleapis/java-firestore/issues/1544)) ([79713bf](https://togithub.com/googleapis/java-firestore/commit/79713bf0fa376a4d22518ae2f5da9660795d9f89))


### Documentation

* Fix formatting due to unclosed backtick ([#1529](https://togithub.com/googleapis/java-firestore/issues/1529)) ([3c78fe3](https://togithub.com/googleapis/java-firestore/commit/3c78fe3c248cb212c6e4f91a5bb7aeb8b9b003b0))
* Rring BulkWriter out of BetaApi status. ([#1513](https://togithub.com/googleapis/java-firestore/issues/1513)) ([c2812f7](https://togithub.com/googleapis/java-firestore/commit/c2812f7cb72257512ffecc98ec1bdb1109d7d044))

---
This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
@release-please release-please bot mentioned this pull request May 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ehsannas ehsannas ehsannas approved these changes

Assignees

@tom-andersen tom-andersen

Labels

api: firestore Issues related to the googleapis/java-firestore API. size: m Pull request size is medium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tom-andersen @ehsannas @yoshi-kokoro