Juanny g - transactions global session ver #2839
Conversation
Still lots of questions...
* Maintain state of session when entering and exiting transaction * Update any collection methods used in mongoengine that accept session * Update any database methods used in mongoengine that accept session * Cascades should be accounted for as those methods are wrappers which call the same low-level pymongo colletion APIs updated above * Any signals that try to read or write should also be wrapped in the transaction
* Mongo 3 doesn't support transactions * Mongo 4 does, but 4.4 specifically started allowing colleciton creation in a transaction
This update accounts for the fact that at least one workflow runs against Mongo 4.0.23
pymongo added support for transactions in 3.7
Use an extension of `threading.locals` to handle initialized sessions within a thread to ensure session isolation from other threads. Also, given that pymongo supports nested sessions + nested transactions, I added a deque to track the "current" session state and mimic this behavior.
pymongo documentation explicitly states the callback of `with_transaction` should not start new transactions
It's not a context manager anymore!
Not a context mgr anymore
W/out --verbose argument, mongo4 would throw errors. Could not reproduce locally and this was the only thing that would consistently work in the github action.
…nyG-transactions-global-session-ver
…actionCommitResult
bagerard
force-pushed
the
juannyG-transactions-global-session-ver
branch
2 times, most recently
from
3b91f90 to
7a61e1c
Compare
bagerard
force-pushed
the
juannyG-transactions-global-session-ver
branch
from
7a61e1c to
8feba86
Compare
|
@juannyG Please have a look at this one, it builds on top of your PR. I've commented the parts I reworked to ease your review, if all looks good to you, it will finally be good to go 🚀 |
|
For future ref, one of the job failed with the famous intermittent "TransientTransactionError" https://github.com/MongoEngine/mongoengine/actions/runs/10708253155/job/29690122622#step:6:395 I suggest to move forward anyway and do any rework on top once merged (to avoid blocking your work any longer) |
Yeah - I saw that 😅. But as we now know, the tests are just like any other app and will have to have something baked in to handle that error. I'll start working on making sure the tests aren't flaky after we get the merge through 💪 Looks good! Nice catch with ensuring the commit goes through! |
|
Finally merged 🚀 |
Rework on top of original PR (#2569) with minor additional commits