> On June 6, 2018, 10:34 p.m., Alexander Kolbasov wrote: > > Looks good, a few nits below.
Thanks for looking into this review. I fixed/answered the issues. Please let me know if the patch looks ok, then I will upload it to the Jira to run the pre-commit tests. > On June 6, 2018, 10:34 p.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Line 3227 (original), 3322 (patched) > > <https://reviews.apache.org/r/66667/diff/2/?file=2012314#file2012314line3325> > > > > Is it possible to do it once in constructor instead? I suspect that > > this is a no-trivial operation. To be honest, I don't see clearly if it would be worth to move this part to the constructor. I am not sure what side effect it would have. In HIVE-15137, where this part was added to the code, the problem was that if two HiveCli were started with different users and both users added a partition, the owner of the partition directories was always the first user. Would moving this code to the constructor not affect this use-case? Would it work correctly? I think, this should be investigated. I am just not sure of the benefit of moving this code. The current user is fetched only once when creating a batch of partitions, and I don't see this as a very expensive call. If we want to move this, I would suggest to investigate and do it in a seperate Jira. What do you think? > On June 6, 2018, 10:34 p.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Lines 3253 (patched) > > <https://reviews.apache.org/r/66667/diff/5/?file=2034474#file2034474line3253> > > > > Can you clarify that "clean up" means removing associated directory. I fixed it accordingly. > On June 6, 2018, 10:34 p.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Lines 3268 (patched) > > <https://reviews.apache.org/r/66667/diff/5/?file=2034474#file2034474line3268> > > > > Please add a Javadoc here explaining what is checked by validation. > > Also it isn't obvious that validation has side effects (updating partsToAdd) Added Javadoc > On June 6, 2018, 10:34 p.m., Alexander Kolbasov wrote: > > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Line 3247 (original), 3343 (patched) > > <https://reviews.apache.org/r/66667/diff/5/?file=2034474#file2034474line3346> > > > > addedPartitions is not defined here so it isn't obvious that it should > > be thread-safe. Is it possible to allocate and return addedPartitions here > > so that you guarantee using of thread-safe map? > > > > Another way you can do it is to collect added partitions in thread-safe > > local map and then copy it to the resulting map once you are done with > > concurrent part. The createPartitionFolders method is called with a ConcurrentHashMap, I thought it would do the trick. Returning with the addedPartitions map would be complicated as we have to return the newParts list as well. So I fixed this issue by introducing a local map and then copy the result to the addedPartitions map. - Marta ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66667/#review203099 ----------------------------------------------------------- On June 11, 2018, 11:27 a.m., Marta Kuczora wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66667/ > ----------------------------------------------------------- > > (Updated June 11, 2018, 11:27 a.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 > b9f5fb8 > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java > bf559b4 > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java > 4f11a55 > > > Diff: https://reviews.apache.org/r/66667/diff/6/ > > > Testing > ------- > > > Thanks, > > Marta Kuczora > >
