----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22356/#review45218 -----------------------------------------------------------
most of it looks good. thanks for great work. Have a closer look at the Connection conn = tx.getConnection() statements These should be solved more elegant/robust. And remove the commits that might be partially. They defeat the purpose of transactions. engine/schema/src/com/cloud/upgrade/DatabaseIntegrityChecker.java <https://reviews.apache.org/r/22356/#comment79944> a commit in a finally when try-with-resource used seems not what you want. What do you expect to be committed here? engine/schema/src/com/cloud/upgrade/DatabaseIntegrityChecker.java <https://reviews.apache.org/r/22356/#comment79945> put in the try() clause if it needs closing engine/schema/src/com/cloud/upgrade/DatabaseIntegrityChecker.java <https://reviews.apache.org/r/22356/#comment79946> use CloudRuntimeException(String msg, Exception e) if you want the pass root cause. engine/schema/src/com/cloud/upgrade/DatabaseIntegrityChecker.java <https://reviews.apache.org/r/22356/#comment79947> spelling: premium(g) engine/schema/src/com/cloud/upgrade/DatabaseIntegrityChecker.java <https://reviews.apache.org/r/22356/#comment79948> use CloudRuntimeException(msg,e) engine/schema/src/com/cloud/upgrade/DatabaseIntegrityChecker.java <https://reviews.apache.org/r/22356/#comment79949> a commit in a finally means you might partialy commit. That is not what you want. engine/schema/src/com/cloud/upgrade/DatabaseIntegrityChecker.java <https://reviews.apache.org/r/22356/#comment79950> find another solution for close in the finally: in the try() or outside the block! framework/db/src/com/cloud/utils/crypt/EncryptionSecretKeyChanger.java <https://reviews.apache.org/r/22356/#comment79951> some better naming for the second statement is a welcome luxury;) framework/db/src/com/cloud/utils/db/TransactionLegacy.java <https://reviews.apache.org/r/22356/#comment79952> maybe you want to put type.equals(item.type) and some extra check to prevent NPEs? and use a similar construct for item.ref? server/src/com/cloud/test/IPRangeConfig.java <https://reviews.apache.org/r/22356/#comment79953> Is Connection not Closable? server/src/com/cloud/test/IPRangeConfig.java <https://reviews.apache.org/r/22356/#comment79955> good riddens :) - daan Hoogland 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 > >
