DRIVERS-2584 Errors handling in Convenient Transactions API #1475
Conversation
source/transactions-convenient-api/transactions-convenient-api.rst
Outdated
Show resolved
Hide resolved
source/transactions-convenient-api/transactions-convenient-api.rst
Outdated
Show resolved
Hide resolved
source/transactions-convenient-api/transactions-convenient-api.rst
Outdated
Show resolved
Hide resolved
….rst Co-authored-by: Andreas Braun <alcaeus@users.noreply.github.com>
|
The underlying issue of suppressing command errors is also a pitfall in the core transaction API. Should we update that spec too? |
Good point, thank you! Added some clarification to the core API. |
source/transactions/transactions.rst
Outdated
| Handling command errors | ||
| ----------------------- | ||
|
|
||
| Drivers MUST document that command errors that do not have the |
There was a problem hiding this comment.
I think this should just be "command errors may abort the transaction on the server", regardless of the label. the server also aborts the transaction on many (if not all) transient transaction errors (e.g. WriteConflict).
There was a problem hiding this comment.
Should we recommend re-raising all command errors inside with_transaction callback then? Since we do not have a reliable indicator which command aborts the transaction and with does not.
There was a problem hiding this comment.
I guess the distinction is perhaps relevant in that, if the user doesn't re-raise an error but it's a TransientTransactionError, the driver still ends up doing the correct thing and retrying, since it gets NoSuchTransaction, a different TransientTransactionError.
whereas for a non-transient error we end up with the infinite loop issue.
so I think it's less strictly necessary to recommend re-raising TransientTransactionErrors. however, the reason it happens to work in that case is a little funny. so for simplicity maybe it's best to not distinguish so users don't have to think about error labels and just say re-raise all errors?
There was a problem hiding this comment.
Good point, I agree. We should recommend re-raising all errors. If a user has some specific use case, core API will be a better choice.
| will result in an infinite loop. | ||
|
|
||
|
|
||
| Drivers SHOULD recommend that the callback re-throw command errors if they |
There was a problem hiding this comment.
Just out of curiosity, why are these a SHOULD and not a MUST? It seems to me like it would be required, 1) to lower the potential cases and 2) to have a quick point of reference for all drivers when users run into this.
There was a problem hiding this comment.
Sounds reasonable, changed to MUST.
source/transactions-convenient-api/transactions-convenient-api.rst
Outdated
Show resolved
Hide resolved
….rst Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
| For example, ``DuplicateKeyError`` is an error that aborts a transaction on the | ||
| server. If the callback catches ``DuplicateKeyError`` and does not re-throw it, | ||
| the driver will attempt to commit the transaction. The server will reject the | ||
| commit attempt with ``NoSuchTransaction`` error. This error has the |
There was a problem hiding this comment.
There was a problem hiding this comment.
We discussed this with @alcaeus last week, and decided to keep the existing commit logic.
Rationale: when a driver receives NoSuchTransaction, we do not know what happened. This situation may be a result of some server side situation, like a fail-over. If this is the case, the driver should retry the transaction completely.
Since we cannot distinguish between situations where the callback did something unexpected and where something went wrong on the server, we should try to be safe, and retry. With the changes proposed in this PR we give users clear instructions to let the driver decide how to handle errors.
Please complete the following before merging:
Ruby Implementation