Alpha162 commented on issue #13905: URL: https://github.com/apache/cloudstack/issues/13905#issuecomment-6413549999
@DaanHoogland #13925 fixes the imbalance, but I wouldn't backport it as it stands. `TransactionLegacy.rollback()` isn't nesting-aware, as the review thread on #13925 notes: it strips every `START_TXN` off the stack and does one physical rollback. So when `persist()` fails inside a transaction the caller already started, the new `finally` rolls back the caller's work too. The caller's later `commit()` then logs "Commit called when it is not a transaction", and anything it does after catching the exception runs outside a transaction. The callers that hit this are the ones that catch `EntityExistsException` inside their own transaction and carry on, which is how this issue started. In `UsageManagerImpl.parse()`, one duplicate helper insert would roll back the processed flags and helper rows of the failing event and every event before it in the batch, and the rest of the batch would then commit row by row. I'd make the failure path nesting-aware. If `persist()` opened the outermost transaction, roll back as #13925 does. If it's nested, unwind only its own level, so the failed insert is a no-op and the caller's transaction carries on. `setSavepoint()` and `rollback(Savepoint)` already do that, though nothing else calls them today, so it'd need unit tests for both the nested and outermost cases. With that in place, yes, 4.20 makes sense. #13926 uses the same approach in seven sibling methods, and it has conflicts now. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
