----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27847/#review60741 -----------------------------------------------------------
Mostly looks good. Few comments. metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java <https://reviews.apache.org/r/27847/#comment102128> currently this method has fragemented and repeated logic. I think it might be easier to read if its written in following way: public MetaStoreDirectSql(PersistenceManager pm) { isMySql = executeDBspecificQuery(mySqlQuery); isOracle = executeDBspecificQuery(oracleQuery); } executeDBSpecificQuery(String sql) { Transaction tx = pm.currentTransaction(); tx.begin(); boolean success = false; try { executeNoResult(query); success = true; } catch () { LOG.debug("Oracle check failed, assuming we are not on oracle: " + t.getMessage()); tx.rollback(); tx = pm.currentTransaction(); tx.begin(); return false; } } I think you get the idea :) metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java <https://reviews.apache.org/r/27847/#comment102130> Do we need query.closeAll() ? metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java <https://reviews.apache.org/r/27847/#comment102129> can be renamed to sqlResult metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java <https://reviews.apache.org/r/27847/#comment102131> Do we need query.close(sqlResult2); - Ashutosh Chauhan On Nov. 11, 2014, 12:23 a.m., Sergey Shelukhin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27847/ > ----------------------------------------------------------- > > (Updated Nov. 11, 2014, 12:23 a.m.) > > > Review request for hive. > > > Repository: hive-git > > > Description > ------- > > see jira > > > Diffs > ----- > > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java > b6c633c > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > 2aa5d20 > > metastore/src/java/org/apache/hadoop/hive/metastore/StatObjectConverter.java > 0f99cf3 > > Diff: https://reviews.apache.org/r/27847/diff/ > > > Testing > ------- > > > Thanks, > > Sergey Shelukhin > >
