fix: Make transaction rollback best effort.

[tom-andersen]

Internal tracking: b/316023452

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.

[ehsannas]

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?

[tom-andersen]

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.

[ehsannas]

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

[ehsannas]

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

Let's do this then

[tom-andersen]

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.

[ehsannas]

SGTM. Thanks

[ehsannas]

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

Let's do this then