> 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?
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. > On June 10, 2014, 12:20 p.m., daan Hoogland wrote: > > engine/schema/src/com/cloud/upgrade/DatabaseIntegrityChecker.java, line 250 > > <https://reviews.apache.org/r/22356/diff/2/?file=606364#file606364line250> > > > > a commit in a finally means you might partialy commit. That is not what > > you want. As mentioned above. > On June 10, 2014, 12:20 p.m., daan Hoogland wrote: > > server/src/com/cloud/test/IPRangeConfig.java, line 377 > > <https://reviews.apache.org/r/22356/diff/2/?file=606370#file606370line377> > > > > Is Connection not Closable? > On June 10, 2014, 12:20 p.m., daan Hoogland wrote: > > framework/db/src/com/cloud/utils/db/TransactionLegacy.java, line 794 > > <https://reviews.apache.org/r/22356/diff/2/?file=606369#file606369line794> > > > > maybe you want to put type.equals(item.type) and some extra check to > > prevent NPEs? > > and use a similar construct for item.ref? Added this check for item.ref as well. > On June 10, 2014, 12:20 p.m., daan Hoogland wrote: > > framework/db/src/com/cloud/utils/crypt/EncryptionSecretKeyChanger.java, > > line 269 > > <https://reviews.apache.org/r/22356/diff/2/?file=606366#file606366line269> > > > > some better naming for the second statement is a welcome luxury;) yeah, it seems little misnomer, changed :) - Santhosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22356/#review45218 ----------------------------------------------------------- On June 10, 2014, 4:43 a.m., Santhosh Edukulla wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22356/ > ----------------------------------------------------------- > > (Updated June 10, 2014, 4: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 > >
