Only retry transaction once #2604
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alcaeus
force-pushed
the
transactions/retry-once
branch
from
ad43cf2 to
e756e3f
Compare
alcaeus
commented
Jan 9, 2024
Comment on lines
+3184
to
+3186
| * This following method was taken from the MongoDB Library and adapted to not use the default 120 seconds timeout. | ||
| * The code within this method is licensed under the Apache License. Copyright belongs to MongoDB, Inc. | ||
| */ |
There was a problem hiding this comment.
Not sure what the best course of action is here, but since the code in question comes from the apache-2.0-licensed MongoDB library and the ODM is MIT licensed, we do need some kind of license and copyright information. Let me know your thoughts on this.
There was a problem hiding this comment.
I think it's OK but I'm not a lawyer :) Maybe you could ask internally if it's sufficient?
| { | ||
| $this->dm->getClient()->selectDatabase('admin')->command([ | ||
| 'configureFailPoint' => 'failCommand', | ||
| // Trigger the error twice, working around retryable writes |
There was a problem hiding this comment.
While testing this change, I noticed that with a single retry all other tests started failing. Upon further investigation, it turns out that using an additional failure to account for retryable writes isn't necessary. Quoting from the Retryable Writes Documentation:
The write operations inside the transaction are not individually retryable, regardless of value of retryWrites.
With that in mind, we don't need to account for this and a single failure is sufficient for this fail point (except for the one test where we want multiple failures to specifically test that the operation is only retried once).
Merged
alcaeus
added a commit
that referenced
this pull request
Jan 19, 2024
* Run CI workflows on feature branches * Refactor commit logic (#2580) * Add tests for commit consistency showing wrong behaviour * Clear scheduled document changes at the end of a commit operation * Rename private variables to communicate intent * Remove obsolete comment * Use different error code * Use single mongos in consistency tests This ensures that the fail points are created on the same server that the write operations take place on, which can't be guaranteed in a sharded cluster with multiple mongoses. * Rename test methods for clarity * Explain reasoning for index error * Extract helper method to create failpoint * Add configuration setting for transactional flush (#2587) * Add configuration setting for transactional flush * Use classic setters for transactional flush setting * Add logic for transactional commits (#2589) * Extract commit logic * Support transactional commit operations * Always use transactional flush if supported With this commit, all tests using the document manager use transactional flush as long as transactions are supported. Certain tests can use the static $allowsTransactions variable to disable this behaviour. * Test with MongoDB 7.0 * Update test names * Update phpstan baseline * Fix query selection in shard key tests * Flip transaction options constant by default * Use supportsTransaction method when skipping tests * Apply review feedback to tests * Add separate test to check write concern in commit options * Strip write options when in transaction * Use majority write concern in test * Update events to play nice with transactions (#2594) * Pass session and transaction information to event args * Only dispatch lifecycle events once per commit operation * Remove isInTransaction property in event args * Split method signature for readability * Use property promotion for event args classes * Extract construction of eventArgs * Inline spl_object_hash calls * Avoid injecting test instance * Add session to commitOptions in persister * Add session assertions in LifecycleEventManager * Only retry transaction once (#2604) * Only retry transaction once * Rename variable * Update transaction documentation (#2606) * Remove references to transactions where not applicable * Update transaction documentation * Apply suggestions from code review Co-authored-by: Jeremy Mikola <[email protected]> * Change title level in event documentation * Add documentation about transactions to event docs --------- Co-authored-by: Jeremy Mikola <[email protected]> * Address code review items * Test highest dependencies on MongoDB 7.0 * Regenerate psalm baseline to silence unrelated errors --------- Co-authored-by: Jeremy Mikola <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a modified implementation of MongoDB's Convenient Transaction API specification that is better suited for our use case. When encountering transient errors, the convenient API will retry the transaction for up to 120 seconds. The only way to change this time limit is by using the
timeoutMSoption specified in the Client Side Operations Timeout (CSOT) specification. However, libmongoc and thus by extension the PHP driver do not yet implement this specification, so users have no way of reducing the timewith_transactionwill potentially take. Since 120 seconds is not a good default timeout in a web context and users of the ODM will most likely want to abort sooner on transient errors, a custom implementation is needed.The implementation in this PR takes the entire logic of the convenient transaction API, but only allows the closure to be run twice, meaning that transient command errors will lead to a single retry. For the time being, the amount of retries is not configurable, but this can be added later. I would like to avoid introducing a config setting unless absolutely needed so that we remain flexible to switch back to using the API provided by the driver once it implements the CSOT specification.