> On April 18, 2018, 9:52 p.m., Alexander Kolbasov wrote: > >
Thanks a lot Sasha for the review! > On April 18, 2018, 9:52 p.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Lines 3303 (patched) > > <https://reviews.apache.org/r/66667/diff/1/?file=2004741#file2004741line3303> > > > > Since you are doing the refactoring, would you mind adding good Javadoc > > to this method? You are right, I added Javadoc to the method. > On April 18, 2018, 9:52 p.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Lines 3308 (patched) > > <https://reviews.apache.org/r/66667/diff/1/?file=2004741#file2004741line3308> > > > > 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. I agree that the others should be still cleaned up. But on the other hand we still should throw an exception because we should notify the user that something went wrong with the folder clean up, so the user could check and make sure that all folders are deleted properly. Would you mind creating a separate Jira about this issue? > On April 18, 2018, 9:52 p.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Lines 3340 (patched) > > <https://reviews.apache.org/r/66667/diff/1/?file=2004741#file2004741line3342> > > > > Please add Javadoc for this method. > > Also I don't see the result used anywhere - can it be changed to void? Sure, I added Javadoc to the method. The result is used in the add_partitions_core method: newParts.addAll(createPartitionFolders(partitionsToAdd, tbl, addedPartitions)); The add_partitions_core and add_partitions_pspec_core methods were a bit different here. The add_partitions_core method has a list (newParts) which was populated like this for (Future<Partition> partFuture : partFutures) { Partition part = partFuture.get(); if (part != null) { newParts.add(part); } } And then this list was used when creating the notification events. In the add_partitions_pspec_core method however there is no such list. It uses the PartitionSpecProxy instead. I didn't want to change the behavior of the methods in the scope of this Jira. > On April 18, 2018, 9:52 p.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Line 3293 (original), 3365 (patched) > > <https://reviews.apache.org/r/66667/diff/1/?file=2004741#file2004741line3368> > > > > 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. I am not sure what you mean. I've only seen this method like this so far. As far as I see, in the MetaStoreUtils.updateBasicState the properties from EnvironmentContext are considered: public static void updateBasicState(EnvironmentContext environmentContext, Map<String,String> params) { if (params == null) { return; } if (environmentContext != null && environmentContext.isSetProperties() && StatsSetupConst.TASK.equals(environmentContext.getProperties().get( StatsSetupConst.STATS_GENERATED))) { StatsSetupConst.setBasicStatsState(params, StatsSetupConst.TRUE); } else { StatsSetupConst.setBasicStatsState(params, StatsSetupConst.FALSE); } } But I am not familiar with stats computing, so you might look for something else. Could you please open a separate Jira about this? > On April 18, 2018, 9:52 p.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Line 3323 (original), 3396 (patched) > > <https://reviews.apache.org/r/66667/diff/1/?file=2004741#file2004741line3399> > > > > Should we set interrupted flag on the thread if we get > > InterruptedException? Could you please give me some details about why you think it is needed? I don't know actually if it is needed or not. My idea here was to go through on all FutureTasks and if one of them didn't finish successfully (there was either an error or the task was interrupted), throw an exception, cause it would mean that not all partition folders were created successfully. For this I don't think that I should set anything on the thread, but I might miss something. So could you please explain me your thoughts on this? > On April 18, 2018, 9:52 p.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Line 3500 (original), 3525 (patched) > > <https://reviews.apache.org/r/66667/diff/1/?file=2004741#file2004741line3576> > > > > Style nit: validPartition doesn't add any value here, why not just > > > > if (validatePartition(part, catName, tblName, dbName, > > partsToAdd, ms, ifNotExists)) {... } Fixed it. > On April 18, 2018, 9:52 p.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Line 3595 (original), 3548 (patched) > > <https://reviews.apache.org/r/66667/diff/1/?file=2004741#file2004741line3671> > > > > Note that cleanupPartitionFolders() here may throw an exception, thus > > preventing other cleanup. I guess you mean the same issue here than in your previous comment: "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." I would close this issue and continue the discussion under the other comment, just to have it in one place. If you meant something else, please feel free to reopen this issue. - Marta ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66667/#review201465 ----------------------------------------------------------- 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 > >
