> 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
> 
>

Reply via email to