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: Make rollback best effort. #1515

Merged
merged 3 commits into from
Jan 18, 2024
Merged

fix: Make rollback best effort. #1515

merged 3 commits into from
Jan 18, 2024

Conversation

tom-andersen
Copy link
Contributor

@tom-andersen tom-andersen commented Jan 4, 2024

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 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
Copy link
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).

Comment on lines 1200 to 1201
System.out.println("BAD INVOCATION");
System.out.println(invocationOnMock.getArguments()[0]);
Copy link
Contributor

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
Contributor Author

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 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
Contributor Author

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 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
23 checks passed
@tom-andersen tom-andersen deleted the tomandersen/b/318686162 branch January 18, 2024 15:43
gcf-merge-on-green bot pushed a commit that referenced this pull request Jan 25, 2024
🤖 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/java-firestore API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants