On Fri, Jul 10, 2026 at 5:13 PM Mark Thomas <[email protected]> wrote: > > On 10/07/2026 13:17, [email protected] wrote: > > This is an automated email from the ASF dual-hosted git repository. > > > > rmaucher pushed a commit to branch main > > in repository https://gitbox.apache.org/repos/asf/tomcat.git > > > > > > The following commit(s) were added to refs/heads/main by this push: > > new 8120a12a0d Fixes from code review > > 8120a12a0d is described below > > > > commit 8120a12a0dbbaafc104fa0d61a319d480b7ff92c > > Author: remm <[email protected]> > > AuthorDate: Fri Jul 10 14:17:44 2026 +0200 > > > > Fixes from code review > > > > Take advantage of rollback to improve save reliability. > > Clear out transient memory changes only if save is successful, > > Line 1253: There is no rollback if the commit fails
I don't see the point. What should I do then if rollback fails. And so on. LLMs often go on a "what if it fails" loop and there's no good solution. > Line 1255-67: Clearing out of transient changes can still fail. Better > to wrapp all the changes in a single transaction that either suceeds or > fails. The idea is to clear out the transient changes if the commit fails. Since there's only one connection to a datasource, I don't see the point of using a transaction, I either commit at the end or rollback. > Lines 1288-1717: Will any of these excetions be thrown no everything is > in a transaction? Assuming the code is correct, I suspect not. This code > could be a lot cleaner. It looks ok to me. The code review indicated that all the errors were logged and everything was going on as is. So now all the exceptions are aggregated and presumably there's a rollback at the end. It's also possible to try to exit early (simply return a single exception ?). > Line 1731: Some of these fields were optional (and are still marked as > such in the documentation). That migth be a breaking change for some odd > configurations. No objections to the change if a) the breaking change is > documented in the migration guide and b) the docs are updated Ok, I see userRoleTable and roleNameCol should be semi optional (so users without roles). I will make some adjustments. Rémy > Mark > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
