> On June 10, 2014, 12:20 p.m., daan Hoogland wrote: > > engine/schema/src/com/cloud/upgrade/DatabaseIntegrityChecker.java, line 136 > > <https://reviews.apache.org/r/22356/diff/2/?file=606364#file606364line136> > > > > a commit in a finally when try-with-resource used seems not what you > > want. What do you expect to be committed here? > > Santhosh Edukulla wrote: > Mentioned transaction logic mentioned seems to be our own implementation. > I have seen changing these commits\close at places leads to issues. Though > connection is getting auto closed, "assuming" a transaction can have multiple > connections and especially shared with scope across process, transaction > commit seems ok. So, earlier i didn't delved in too much details, > transaction start and commit seems reasonable, went to keep it like that. > > There are few places where this piece of code was used. With this fix, I > touched to see where leaks are there, and fixed. Went conservative at places, > cautious not to introduce regression issues. If we agree, i will still keep > it like this. Changing these at all places, may be like like additional > refactoring, making sure our own implementation of transaction works\breaks > and require thorough testing. Let me know your thoughts. > > daan Hoogland wrote: > I certainly don't agree. A transaction that does an unconditional commit > in a finally makes no sense. If the problem is that our Transaction doesn't > implement Closable we should make it implement that interface. It already > implements close() so this is no big change. At the end of regular execution > the commit should be done, not in the finally. The exception handlers should > do a rollback(). > > I realize that this is not entirely new in your code but it is a big bug > and as you do touch the contents of the finally clause, please fix. (A little > fix I think) > > Santhosh Edukulla wrote: > Ok, in general this is what this behavior should be. Anyways,below are > what we wanted. > > 1. So, move txn commit to end of try block and retain txn close in > finally? > 2. Regarding rollback, do we need rollback in outer catch? Again, > rollback can lead to exception, so try\catch again? once it is in outer > catch, the required resources to rollback are anyways autoclosed or not > available for rolling back anything? so, rollback is not required in > catch?yes\no? > > daan Hoogland wrote: > ad 1. agree > ad 2. rollback yes, but rollback should be automatic when no commit is > done, if rollback fails nothing else can be done. Our Transaction class must > implement Closable so the close call is implicit and the opening can go in a > try-with-resource clause
so actually ad 1. should be [partially agree - daan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22356/#review45218 ----------------------------------------------------------- On June 11, 2014, 6:43 a.m., Santhosh Edukulla wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22356/ > ----------------------------------------------------------- > > (Updated June 11, 2014, 6:43 a.m.) > > > Review request for cloudstack and daan Hoogland. > > > Repository: cloudstack-git > > > Description > ------- > > Fixed few coverity issues reported for resource leak, value comparison, > invalid loop check for result set. > > > Diffs > ----- > > engine/schema/src/com/cloud/upgrade/DatabaseCreator.java 91ef318 > engine/schema/src/com/cloud/upgrade/DatabaseIntegrityChecker.java c20a418 > engine/schema/src/com/cloud/upgrade/DatabaseUpgradeChecker.java 0761c9f > framework/db/src/com/cloud/utils/crypt/EncryptionSecretKeyChanger.java > 58584f9 > framework/db/src/com/cloud/utils/db/Merovingian2.java 6eeea9f > framework/db/src/com/cloud/utils/db/ScriptRunner.java 6614527 > framework/db/src/com/cloud/utils/db/TransactionLegacy.java ac0ea21 > server/src/com/cloud/test/IPRangeConfig.java 1d56471 > usage/src/com/cloud/usage/UsageSanityChecker.java 5e6123b > utils/src/com/cloud/utils/crypt/EncryptionSecretKeySender.java 086e8a8 > > Diff: https://reviews.apache.org/r/22356/diff/ > > > Testing > ------- > > 1.Built the code and found no issues. > 2.Built the simulator and ran a deploy datacenter with the changes. > > > Thanks, > > Santhosh Edukulla > >
