Wrap OrderUpdater#update in a transaction #1569
Conversation
As of solidusio#1479 we are doing more work inside of OrderUpdater. We're doing additions and deletions of tax adjustments, which also causes more touching of records. This is all made faster by wrapping the entire update in a transaction. All the writes can be queued together by the DB, and as of rails 5, dependent touching is grouped together to the end of the transaction.
|
Nice! The optimizations look great! I'm trying to think through the consequences of having one big transaction for this. E.g. the db records from third party tax extensions, which will talk to remote services, may all be rolled back, losing track of things communicated to those services. Any thoughts or suggestions on how we handle that and how we message that to people writing these extensions? |
|
@jordan-brough yea, its a bit tough since if we dont rollback, its quite likely orders are in the "wrong" state for other reasons. Maybe we should consider some kind of datastore which would be responsible for handling certain types of records in case of _rollback? |
|
For the record I am 👍 on this, but do echo Jordan's concerns |
|
I'm personally pretty comfortable with any work being rolled back here in the case of an exception. The OrderUpdater really shouldn't be raising exceptions. When it does, the most likely culprit would be issues communicating with a 3rd party tax extension, in which case it wouldn't make sense to store any data. In other cases something must have been invalid, and any stored data would likely be garbage. |
As of #1479 we are doing more work inside of OrderUpdater. We're doing additions and deletions of tax adjustments, which also causes more touching of records.
This is all made faster by wrapping the entire update in a transaction. All the writes can be queued together by the DB, and as of rails 5, dependent touching is grouped together to the end of the transaction.
Difference in queries run:
https://gist.github.com/jhawthorn/5f0b9de70a782bd3e1fd2ecec667e28c