In the Basic tracker plugin, we use database transaction in several places, rolling back the transaction if an exception occurs. However, noticing this again after a while, I’m now thinking about whether this actually makes sense?
First off, this only makes a difference in about 0.1% of the cases, namely when there are more than 1,000 IDs to track, the first batch succeeded and one of the subsequent batches then fails. Otherwise, you roll back a transaction with zero (successful) changes in it.
(Worst is in trackAllItemsDeleted(), where we always just execute a single DELETE query – I don’t really see any way that a rollback there would actually have any effect.)
But even if we hit that case and there is something to roll back: The exception is in all cases logged, not propagated, so execution just continues normally and the tracking operation will not be re-tried or anything. The user would have to manually re-index all items (or rebuild the tracker, depending on which tracking operation failed) for this to be fixed.
That being the case, the only effect of the rollback is that more items will be affected, having eliminated the correct tracking operation for the first batch(es) where it was successful. Rollbacks are used to avoid ending up in an inconsistent state, but this is not the case here – with or without the rollback, the state will be “inconsistent“ (as in: the tracker doesn’t correctly reflect the state of some items), but not fatally so (no error should occur anywhere afterwards – search results might just be incomplete/off), and the rollback just increases the damage (the number of incorrect items).
Would be great to get feedback on this, but typing this out I’m already pretty convinced that removing those transactions is the way to go, I think. Still, will wait a bit for any input.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 3198412-6--remove_rollbacks_in_tracker_methods.patch | 5.8 KB | drunken monkey |
Comments
Comment #2
drunken monkeyThis would implement the discussed change. Feedback still very much welcome!
Comment #3
drunken monkeyOops.
Comment #5
drunken monkeyMakes sense.
Comment #6
drunken monkeyRe-roll.
Comment #8
drunken monkeyWell, as not even the mention in the release notes has lead to any feedback, I’m gonna go ahead and commit this and, as usual, wait for someone to complain I broke their site.
Comment #9
artusamakJust an indirect feedback so that you keep faith in the community and writing great changelogs as you did! And it's a good excuse to thank you for your great work with SAPI!
Your conclusion make sense (and yes, people will complain only if it breaks ;-)). Another approach to this feature would be to consider that it's not a transaction since the batch process breaks it but a queue instead? A queue of items to update and with a process to retry if the indexing fails?
Just my 2 cents, hopes it can help. ;-)
Comment #10
drunken monkeyThanks for the feedback, very nice of you! ;)
Yes, including some retry functionality, like we have with some other tasks in the module, would be the “proper” solution here, most likely. However, as we’re just talking about pretty simple DB statements failing, which will (I think) only happen very rarely, I don’t think it’s worth the trouble at this point.
So, let’s just keep it for now and wait whether something pops up.
In any case, thanks again!