> 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)
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? - Santhosh ----------------------------------------------------------- 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 > >
