> On Feb. 21, 2018, 1:38 p.m., Peter Vary wrote:
> > Thanks for the patch Marta!
> > Mostly just questions.
> > Peter

Thanks a lot Peter for the review.


> On Feb. 21, 2018, 1:38 p.m., Peter Vary wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 2830 (patched)
> > <https://reviews.apache.org/r/65716/diff/1/?file=1962881#file1962881line2830>
> >
> >     nit: formatting

Thanks, I'll fix it. It seems the autoformat is not perfect either. :)


> On Feb. 21, 2018, 1:38 p.m., Peter Vary wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 2856 (patched)
> > <https://reviews.apache.org/r/65716/diff/1/?file=1962881#file1962881line2856>
> >
> >     Question: Do this has to be AtomicBoolean instead of simple boolean?

It cannot be a simple boolean, cause it leads to compile error: Local variable 
defined in an enclosing scope must be final or effectively final
But the boolean flag cannot be final since it has to be set in case of error. 
So we need some kind of wrapper here and AtomicBoolean seemed a good choice to 
me. Please tell me if you have concerns with AtomicBoolean.


> On Feb. 21, 2018, 1:38 p.m., Peter Vary wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 3098 (patched)
> > <https://reviews.apache.org/r/65716/diff/1/?file=1962881#file1962881line3105>
> >
> >     Same as above

See above.


> On Feb. 21, 2018, 1:38 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Line 840 (original), 858 (patched)
> > <https://reviews.apache.org/r/65716/diff/1/?file=1962882#file1962882line869>
> >
> >     Is this line only changed in formatting?

Yes, just for some reason the diff looks weird.


> On Feb. 21, 2018, 1:38 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java
> > Lines 626 (patched)
> > <https://reviews.apache.org/r/65716/diff/1/?file=1962883#file1962883line629>
> >
> >     Why is this change? Is this an incompatible change?

No it is not an incompatible change. What happened is that we got a NPE in this 
case, but it was inside the task execution, so it got wrapped into a 
MetaException. Since some code was moved outside the task execution, the NPE 
which occurred here was not wrapped any more. But I will modify some parts so a 
MetaException will occur just as before.


- Marta


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65716/#review197843
-----------------------------------------------------------


On Feb. 20, 2018, 5:03 p.m., Marta Kuczora wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65716/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2018, 5:03 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18696
>     https://issues.apache.org/jira/browse/HIVE-18696
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> The idea behind the patch is
> 
> 1) Separate the partition validation from starting the tasks which create the 
> partition folders. 
> Instead of doing the checks on the partitions and submit the tasks in one 
> loop, separated the validation into a different loop. So first iterate 
> through the partitions, validate the table/db names, and check for 
> duplicates. Then if all partitions were correct, in the second loop submit 
> the tasks to create the partition folders. This way if one of the partitions 
> is incorrect, the exception will be thrown in the first loop, before the 
> tasks are submitted. So we can be sure that no partition folder will be 
> created if the list contains an invalid partition.
> 
> 2) Handle the exceptions which occur during the execution of the tasks 
> differently.
> Previously if an exception occured in one task, the remaining tasks were 
> canceled, and the newly created partition folders were cleaned up in the 
> finally part. The problem was that it could happen that some tasks were still 
> not finished with the folder creation when cleaning up the others, so there 
> could have been leftover folders. After doing some testing it turned out that 
> this use case cannot be avoided completely when canceling the tasks.
> The idea of this patch is to set a flag if an exception is thrown in one of 
> the tasks. This flag is visible in the tasks and if its value is true, the 
> partition folders won't be created. Then iterate through the remaining tasks 
> and wait for them to finish. The tasks which are started before the flag got 
> set will then finish creating the partition folders. The tasks which are 
> started after the flag got set, won't create the partition folders, to avoid 
> unnecessary work. This way it is sure that all tasks are finished, when 
> entering the finally part where the partition folders are cleaned up.
> 
> 
> Diffs
> -----
> 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  47de215 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
>  f483ca8 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java
>  919ba78 
> 
> 
> Diff: https://reviews.apache.org/r/65716/diff/1/
> 
> 
> Testing
> -------
> 
> Added some new tests cases to the TestAddPartitions and 
> TestAddPartitionsFromPartSpec tests.
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Reply via email to