----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66667/#review201465 -----------------------------------------------------------
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 3303 (patched) <https://reviews.apache.org/r/66667/#comment282703> Since you are doing the refactoring, would you mind adding good Javadoc to this method? standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 3308 (patched) <https://reviews.apache.org/r/66667/#comment282704> Here we are trying to nuke a bunch of values. If a single one fails, we do not attempt to delete others. Since you are just doing refactoring it is out of scope but I think the proper behavior is to continue nuking for others as well. standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Lines 3340 (patched) <https://reviews.apache.org/r/66667/#comment282705> Please add Javadoc for this method. Also I don't see the result used anywhere - can it be changed to void? standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Line 3293 (original), 3365 (patched) <https://reviews.apache.org/r/66667/#comment282709> This is outside of your fix, but there is something strange going on. Looking at initializeAddedPartition() I see that it does: if (MetastoreConf.getBoolVar(conf, ConfVars.STATS_AUTO_GATHER) && !MetaStoreUtils.isView(tbl)) { MetaStoreUtils.updatePartitionStatsFast(part, tbl, wh, madeDir, false, null, true); } I am pretty sure that earlier there was a check for a setting from environment context that could disable stats gathering and now it is gone. This stats gathering is very expensive and Impala disables it using environmental context settings. standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Line 3323 (original), 3396 (patched) <https://reviews.apache.org/r/66667/#comment282710> Should we set interrupted flag on the thread if we get InterruptedException? standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Line 3500 (original), 3525 (patched) <https://reviews.apache.org/r/66667/#comment282714> Style nit: validPartition doesn't add any value here, why not just if (validatePartition(part, catName, tblName, dbName, partsToAdd, ms, ifNotExists)) {... } standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java Line 3595 (original), 3548 (patched) <https://reviews.apache.org/r/66667/#comment282718> Note that cleanupPartitionFolders() here may throw an exception, thus preventing other cleanup. - Alexander Kolbasov On April 17, 2018, 1:37 p.m., Marta Kuczora wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66667/ > ----------------------------------------------------------- > > (Updated April 17, 2018, 1:37 p.m.) > > > Review request for hive, Peter Vary, Sahil Takiar, and Adam Szita. > > > Bugs: HIVE-19046 > https://issues.apache.org/jira/browse/HIVE-19046 > > > Repository: hive-git > > > Description > ------- > > The biggest part of these methods use the same code. Refactored these code > parts to common methods. > > > Diffs > ----- > > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > ae9ec5c > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java > f8497c7 > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java > fc0c60f > > > Diff: https://reviews.apache.org/r/66667/diff/1/ > > > Testing > ------- > > > Thanks, > > Marta Kuczora > >
