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.

Sign up for GitHub

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 transaction rollback best effort. #1967

Merged
merged 2 commits into from
Jan 5, 2024
Merged

fix: Make transaction rollback best effort. #1967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a36b2ce
Make rollback best effort.
tom-andersen Jan 3, 2024
0861bf7
Fix test
tom-andersen Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion dev/src/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,20 @@ export class Transaction implements firestore.Transaction {
transaction: this._transactionId,
};

return this._firestore.request('rollback', request, this._requestTag);
const promise: Promise<void> = this._firestore.request(
'rollback',
request,
this._requestTag
);

return promise.catch(reason => {
logger(
'Firestore.runTransaction',
this._requestTag,
'Best effort to rollback failed with error:',
reason
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the only question is whether there are rollback failures that should be retried?

maybe MISSING_TRANSACTION_HANDLE should be the only one that's best-effort?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the documentation "Note that a rollback itself might fail, so the rollback should be a best-effort attempt only."

https://cloud.google.com/datastore/docs/best-practices#api_calls

However, I see your point.

This could be improved such that retryable errors from rollback cause rollback to be retried, except INTERNAL which happens when MISSING_TRANSACTION_HANDLE.

BUT, even if rollback experiences a non-retryable error, the transaction should still retry. That was not the case when I looked at Java, which seems completely wrong even if it was baked into a test case.

The phrasing "Best effort" leaves some nuance to be interpreted.

Ideally MISSING_TRANSACTION_HANDLE should just be a no-op on Firestore backend, and NOT throw an INTERNAL error. This code might simply be working around what should be a change to the backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally MISSING_TRANSACTION_HANDLE should just be a no-op on Firestore backend, and NOT throw an INTERNAL error. This code might simply be working around what should be a change to the backend.

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be improved such that retryable errors from rollback cause rollback to be retried, except INTERNAL

Let's do this then

Copy link
Contributor Author

@tom-andersen tom-andersen Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I looked into code, and the underlying GAPIC client does do retries according to retry settings. Effectively, that means currently there is appropriate backoff, and 5 attempts to rollback before failure.

This second level of retry on a higher level was a complete duplication and would simply multiply the number of attempts with additional backoff.

In addition, after some investigation, the GAPIC level retry respects the error codes from service config. So it actually does a better job of determining when to retry than the higher transaction level retry.

In other words, we should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM. Thanks

}

/**
Expand Down
1 change: 0 additions & 1 deletion dev/test/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,6 @@ describe('failed transactions', () => {
begin({transactionId: 'foo1'}),
commit('foo1', /* writes=*/ undefined, serverError),
rollback('foo1', serverError),
rollback('foo1'),
backoff(),
begin({
transactionId: 'foo2',
Expand Down
Loading