You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Marta Kuczora via Review Board <no...@reviews.apache.org> on 2018/02/20 17:03:29 UTC
Review Request 65716: HIVE-18696: The partition folders might not get
cleaned up properly in the HiveMetaStore.add_partitions_core method
if an exception occurs
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65716/
-----------------------------------------------------------
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
Re: Review Request 65716: HIVE-18696: The partition folders might not
get
cleaned up properly in the HiveMetaStore.add_partitions_core method if an
exception occurs
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
> 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
>
>
Re: Review Request 65716: HIVE-18696: The partition folders might not
get
cleaned up properly in the HiveMetaStore.add_partitions_core method if an
exception occurs
Posted by Peter Vary via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65716/#review197843
-----------------------------------------------------------
Thanks for the patch Marta!
Mostly just questions.
Peter
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 2830 (patched)
<https://reviews.apache.org/r/65716/#comment278134>
nit: formatting
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 2856 (patched)
<https://reviews.apache.org/r/65716/#comment278133>
Question: Do this has to be AtomicBoolean instead of simple boolean?
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 3098 (patched)
<https://reviews.apache.org/r/65716/#comment278136>
Same as above
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
Line 840 (original), 858 (patched)
<https://reviews.apache.org/r/65716/#comment278141>
Is this line only changed in formatting?
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java
Lines 626 (patched)
<https://reviews.apache.org/r/65716/#comment278139>
Why is this change? Is this an incompatible change?
- Peter Vary
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
>
>
Re: Review Request 65716: HIVE-18696: The partition folders might not
get
cleaned up properly in the HiveMetaStore.add_partitions_core method if an
exception occurs
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65716/
-----------------------------------------------------------
(Updated March 6, 2018, 5:24 p.m.)
Review request for hive, Peter Vary and Adam Szita.
Changes
-------
Fix some review findings and also added some test cases for adding partitions to view.
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 (updated)
-----
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 2be018b
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java 4d9cb1b
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java 1122057
Diff: https://reviews.apache.org/r/65716/diff/2/
Changes: https://reviews.apache.org/r/65716/diff/1-2/
Testing
-------
Added some new tests cases to the TestAddPartitions and TestAddPartitionsFromPartSpec tests.
Thanks,
Marta Kuczora
Re: Review Request 65716: HIVE-18696: The partition folders might not
get
cleaned up properly in the HiveMetaStore.add_partitions_core method if an
exception occurs
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
> On Feb. 21, 2018, 3:32 p.m., Sahil Takiar wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Line 3032 (original), 3065 (patched)
> > <https://reviews.apache.org/r/65716/diff/1/?file=1962881#file1962881line3072>
> >
> > this code looks very similar to the block above. I know its was never the intention of this JIRA to do any re-factoring, but how difficult would it be to move all this code into a common method so that we don't have to fix the bug in two places? not a blocking issue though
Yeah, I absolutely agree. This code duplication annoys me as well, just I wasn't sure that it is acceptable doing the refactoring in the scope of this Jira. But it is not so difficult, so I will upload a patch where I moved the common parts to a separate method and we can decide if it is ok like that or rather do it in a different Jira.
> On Feb. 21, 2018, 3:32 p.m., Sahil Takiar wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 3149-3150 (patched)
> > <https://reviews.apache.org/r/65716/diff/1/?file=1962881#file1962881line3163>
> >
> > curious what behavior you were seeing, wondering why cancelling or interrupting the `Future`s doesn't work
The idea was to iterate over the Future tasks and call their get method to wait until they finish. If an exception occurred in one task, iterated over the tasks again and canceled them with mayInterruptIfRunning=false flag. According to the Java doc, if this flag is false, the in-progress tasks are allowed to complete. So the idea behind this was to avoid interrupting a task when it already started creating the folder. So the situation when the folder is created, but it is not yet put to the addedPartitions map doesn't happen.
Then wait for the already running tasks to complete and I assumed we are good at that point because all tasks are either finished or didn't even started.
Imagine a flow something like this in code:
boolean failureOccurred = false;
try {
for (Future<Partition> partFuture : partFutures) {
partFuture.get();
}
} catch (InterruptedException | ExecutionException e) {
failureOccurred = true;
}
for (Future<Partition> task : partFutures) {
task.cancel(false);
}
for (Future<Partition> partFuture : partFutures) {
if (!partFuture.isCanceled()) {
partFuture.get();
}
}
Then I created a test to see if it works as I expected. I tried to create 40 partitions and had a counter which were visible to the tasks and threw an exception when it reached 20. What I noticed is that almost every time I got a ConcurrentModificationException on this line
for (Map.Entry<PartValEqWrapperLite, Boolean> e : addedPartitions.entrySet()) {
So there must be some tasks still writing the addedPartitions map at that point. By the way, changing the type of the map to ConcurrentMap proved this right, as no exception occurred in this case, but there were leftover folders.
So I started to debug it, mainly with logging when the call method of a task is called, when a task get canceled and what was the result when the get method was called. What I found that there were tasks which were started, their call method was called, so they started to create the folder, but then there was a successful cancel on them. For these tasks the get method simply would throw a CancellationException as it sees the task is not running any more (or the isCanceled method would return true). But actually these tasks created the folder, but it could happen that they didn't finish until the clean up.
I checked the FutureTask code and the run method checks if the state of the task is NEW and if it is, calls the Callable's call method. But doesn't change the state at that point. My theory is that if a cancel is called on the same task at this point, it will also see that the state is NEW, so it will change it to CANCELLED. So I believe a task can go into a weird state like this.
Calling the cancel with mayInterruptIfRunning=true also resulted the same.
So I didn't find a bullet proof solution with canceling the tasks, but it can be that I missed something and there is a good way to solve this.
If you have any idea, please share it with me, any idea is welcome. :)
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65716/#review197829
-----------------------------------------------------------
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
>
>
Re: Review Request 65716: HIVE-18696: The partition folders might not
get
cleaned up properly in the HiveMetaStore.add_partitions_core method if an
exception occurs
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
> On Feb. 21, 2018, 3:32 p.m., Sahil Takiar wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Line 3032 (original), 3065 (patched)
> > <https://reviews.apache.org/r/65716/diff/1/?file=1962881#file1962881line3072>
> >
> > this code looks very similar to the block above. I know its was never the intention of this JIRA to do any re-factoring, but how difficult would it be to move all this code into a common method so that we don't have to fix the bug in two places? not a blocking issue though
>
> Marta Kuczora wrote:
> Yeah, I absolutely agree. This code duplication annoys me as well, just I wasn't sure that it is acceptable doing the refactoring in the scope of this Jira. But it is not so difficult, so I will upload a patch where I moved the common parts to a separate method and we can decide if it is ok like that or rather do it in a different Jira.
I checked how this could be refactored and there are some differences between the methods which makes it not that straightforward. It is not that difficult and basically I have the patch, but I would do it in the scope of an other Jira, so we can discuss some details there. Would this be ok for you Sahil?
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65716/#review197829
-----------------------------------------------------------
On March 6, 2018, 5:24 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65716/
> -----------------------------------------------------------
>
> (Updated March 6, 2018, 5:24 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 2be018b
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java 4d9cb1b
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java 1122057
>
>
> Diff: https://reviews.apache.org/r/65716/diff/2/
>
>
> Testing
> -------
>
> Added some new tests cases to the TestAddPartitions and TestAddPartitionsFromPartSpec tests.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 65716: HIVE-18696: The partition folders might not
get
cleaned up properly in the HiveMetaStore.add_partitions_core method if an
exception occurs
Posted by Sahil Takiar <ta...@gmail.com>.
> On Feb. 21, 2018, 3:32 p.m., Sahil Takiar wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 3149-3150 (patched)
> > <https://reviews.apache.org/r/65716/diff/1/?file=1962881#file1962881line3163>
> >
> > curious what behavior you were seeing, wondering why cancelling or interrupting the `Future`s doesn't work
>
> Marta Kuczora wrote:
> The idea was to iterate over the Future tasks and call their get method to wait until they finish. If an exception occurred in one task, iterated over the tasks again and canceled them with mayInterruptIfRunning=false flag. According to the Java doc, if this flag is false, the in-progress tasks are allowed to complete. So the idea behind this was to avoid interrupting a task when it already started creating the folder. So the situation when the folder is created, but it is not yet put to the addedPartitions map doesn't happen.
> Then wait for the already running tasks to complete and I assumed we are good at that point because all tasks are either finished or didn't even started.
>
> Imagine a flow something like this in code:
>
> boolean failureOccurred = false;
> try {
> for (Future<Partition> partFuture : partFutures) {
> partFuture.get();
> }
> } catch (InterruptedException | ExecutionException e) {
> failureOccurred = true;
> }
>
> for (Future<Partition> task : partFutures) {
> task.cancel(false);
> }
>
> for (Future<Partition> partFuture : partFutures) {
> if (!partFuture.isCanceled()) {
> partFuture.get();
> }
> }
>
>
> Then I created a test to see if it works as I expected. I tried to create 40 partitions and had a counter which were visible to the tasks and threw an exception when it reached 20. What I noticed is that almost every time I got a ConcurrentModificationException on this line
>
> for (Map.Entry<PartValEqWrapperLite, Boolean> e : addedPartitions.entrySet()) {
>
> So there must be some tasks still writing the addedPartitions map at that point. By the way, changing the type of the map to ConcurrentMap proved this right, as no exception occurred in this case, but there were leftover folders.
>
> So I started to debug it, mainly with logging when the call method of a task is called, when a task get canceled and what was the result when the get method was called. What I found that there were tasks which were started, their call method was called, so they started to create the folder, but then there was a successful cancel on them. For these tasks the get method simply would throw a CancellationException as it sees the task is not running any more (or the isCanceled method would return true). But actually these tasks created the folder, but it could happen that they didn't finish until the clean up.
>
> I checked the FutureTask code and the run method checks if the state of the task is NEW and if it is, calls the Callable's call method. But doesn't change the state at that point. My theory is that if a cancel is called on the same task at this point, it will also see that the state is NEW, so it will change it to CANCELLED. So I believe a task can go into a weird state like this.
> Calling the cancel with mayInterruptIfRunning=true also resulted the same.
> So I didn't find a bullet proof solution with canceling the tasks, but it can be that I missed something and there is a good way to solve this.
>
> If you have any idea, please share it with me, any idea is welcome. :)
hmm this is odd. did you try checking the return value of the `cancel` method, the javadocs say it returns `true` if the cancel was successful and `false` otherwise; is it returning `true`?
I can understand that `mayInterruptIfRunning=true` won't help, because it just causes the thread to throw an `InterruptedException` but any code running in that thread can just catch the exception and drop it
could it be possible the second `partFuture.get()` threw an exception, in which case the `finally` block will be trigerred before all threads complete?
I think the solution you have proposed works fine, but the `Future#cancel` methodology should work too and is slightly cleaner, but if you can't get it to work no worries, don't spend too much time on it
- Sahil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65716/#review197829
-----------------------------------------------------------
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
>
>
Re: Review Request 65716: HIVE-18696: The partition folders might not
get
cleaned up properly in the HiveMetaStore.add_partitions_core method if an
exception occurs
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
> On Feb. 21, 2018, 3:32 p.m., Sahil Takiar wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 3149-3150 (patched)
> > <https://reviews.apache.org/r/65716/diff/1/?file=1962881#file1962881line3163>
> >
> > curious what behavior you were seeing, wondering why cancelling or interrupting the `Future`s doesn't work
>
> Marta Kuczora wrote:
> The idea was to iterate over the Future tasks and call their get method to wait until they finish. If an exception occurred in one task, iterated over the tasks again and canceled them with mayInterruptIfRunning=false flag. According to the Java doc, if this flag is false, the in-progress tasks are allowed to complete. So the idea behind this was to avoid interrupting a task when it already started creating the folder. So the situation when the folder is created, but it is not yet put to the addedPartitions map doesn't happen.
> Then wait for the already running tasks to complete and I assumed we are good at that point because all tasks are either finished or didn't even started.
>
> Imagine a flow something like this in code:
>
> boolean failureOccurred = false;
> try {
> for (Future<Partition> partFuture : partFutures) {
> partFuture.get();
> }
> } catch (InterruptedException | ExecutionException e) {
> failureOccurred = true;
> }
>
> for (Future<Partition> task : partFutures) {
> task.cancel(false);
> }
>
> for (Future<Partition> partFuture : partFutures) {
> if (!partFuture.isCanceled()) {
> partFuture.get();
> }
> }
>
>
> Then I created a test to see if it works as I expected. I tried to create 40 partitions and had a counter which were visible to the tasks and threw an exception when it reached 20. What I noticed is that almost every time I got a ConcurrentModificationException on this line
>
> for (Map.Entry<PartValEqWrapperLite, Boolean> e : addedPartitions.entrySet()) {
>
> So there must be some tasks still writing the addedPartitions map at that point. By the way, changing the type of the map to ConcurrentMap proved this right, as no exception occurred in this case, but there were leftover folders.
>
> So I started to debug it, mainly with logging when the call method of a task is called, when a task get canceled and what was the result when the get method was called. What I found that there were tasks which were started, their call method was called, so they started to create the folder, but then there was a successful cancel on them. For these tasks the get method simply would throw a CancellationException as it sees the task is not running any more (or the isCanceled method would return true). But actually these tasks created the folder, but it could happen that they didn't finish until the clean up.
>
> I checked the FutureTask code and the run method checks if the state of the task is NEW and if it is, calls the Callable's call method. But doesn't change the state at that point. My theory is that if a cancel is called on the same task at this point, it will also see that the state is NEW, so it will change it to CANCELLED. So I believe a task can go into a weird state like this.
> Calling the cancel with mayInterruptIfRunning=true also resulted the same.
> So I didn't find a bullet proof solution with canceling the tasks, but it can be that I missed something and there is a good way to solve this.
>
> If you have any idea, please share it with me, any idea is welcome. :)
>
> Sahil Takiar wrote:
> hmm this is odd. did you try checking the return value of the `cancel` method, the javadocs say it returns `true` if the cancel was successful and `false` otherwise; is it returning `true`?
>
> I can understand that `mayInterruptIfRunning=true` won't help, because it just causes the thread to throw an `InterruptedException` but any code running in that thread can just catch the exception and drop it
>
> could it be possible the second `partFuture.get()` threw an exception, in which case the `finally` block will be trigerred before all threads complete?
>
> I think the solution you have proposed works fine, but the `Future#cancel` methodology should work too and is slightly cleaner, but if you can't get it to work no worries, don't spend too much time on it
Yep, I checked it and for these "leftover" tasks, the cancel returned true. There were tasks for which the cancel returned false, but those were ok, because the get just waited for them.
No, I didn't get exception during the second partFuture.get(). Also when I logged what happens with the tasks, all tasks appeared in this second get loop. For the ones which were canceled successfully (even though they are running), just showed that they were canceled.
I agree that it would be a cleaner solution and I spent quite some time with trying to make it work, but I could make this use case work.
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65716/#review197829
-----------------------------------------------------------
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
>
>
Re: Review Request 65716: HIVE-18696: The partition folders might not
get
cleaned up properly in the HiveMetaStore.add_partitions_core method if an
exception occurs
Posted by Sahil Takiar <ta...@gmail.com>.
> On Feb. 21, 2018, 3:32 p.m., Sahil Takiar wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 3149-3150 (patched)
> > <https://reviews.apache.org/r/65716/diff/1/?file=1962881#file1962881line3163>
> >
> > curious what behavior you were seeing, wondering why cancelling or interrupting the `Future`s doesn't work
>
> Marta Kuczora wrote:
> The idea was to iterate over the Future tasks and call their get method to wait until they finish. If an exception occurred in one task, iterated over the tasks again and canceled them with mayInterruptIfRunning=false flag. According to the Java doc, if this flag is false, the in-progress tasks are allowed to complete. So the idea behind this was to avoid interrupting a task when it already started creating the folder. So the situation when the folder is created, but it is not yet put to the addedPartitions map doesn't happen.
> Then wait for the already running tasks to complete and I assumed we are good at that point because all tasks are either finished or didn't even started.
>
> Imagine a flow something like this in code:
>
> boolean failureOccurred = false;
> try {
> for (Future<Partition> partFuture : partFutures) {
> partFuture.get();
> }
> } catch (InterruptedException | ExecutionException e) {
> failureOccurred = true;
> }
>
> for (Future<Partition> task : partFutures) {
> task.cancel(false);
> }
>
> for (Future<Partition> partFuture : partFutures) {
> if (!partFuture.isCanceled()) {
> partFuture.get();
> }
> }
>
>
> Then I created a test to see if it works as I expected. I tried to create 40 partitions and had a counter which were visible to the tasks and threw an exception when it reached 20. What I noticed is that almost every time I got a ConcurrentModificationException on this line
>
> for (Map.Entry<PartValEqWrapperLite, Boolean> e : addedPartitions.entrySet()) {
>
> So there must be some tasks still writing the addedPartitions map at that point. By the way, changing the type of the map to ConcurrentMap proved this right, as no exception occurred in this case, but there were leftover folders.
>
> So I started to debug it, mainly with logging when the call method of a task is called, when a task get canceled and what was the result when the get method was called. What I found that there were tasks which were started, their call method was called, so they started to create the folder, but then there was a successful cancel on them. For these tasks the get method simply would throw a CancellationException as it sees the task is not running any more (or the isCanceled method would return true). But actually these tasks created the folder, but it could happen that they didn't finish until the clean up.
>
> I checked the FutureTask code and the run method checks if the state of the task is NEW and if it is, calls the Callable's call method. But doesn't change the state at that point. My theory is that if a cancel is called on the same task at this point, it will also see that the state is NEW, so it will change it to CANCELLED. So I believe a task can go into a weird state like this.
> Calling the cancel with mayInterruptIfRunning=true also resulted the same.
> So I didn't find a bullet proof solution with canceling the tasks, but it can be that I missed something and there is a good way to solve this.
>
> If you have any idea, please share it with me, any idea is welcome. :)
>
> Sahil Takiar wrote:
> hmm this is odd. did you try checking the return value of the `cancel` method, the javadocs say it returns `true` if the cancel was successful and `false` otherwise; is it returning `true`?
>
> I can understand that `mayInterruptIfRunning=true` won't help, because it just causes the thread to throw an `InterruptedException` but any code running in that thread can just catch the exception and drop it
>
> could it be possible the second `partFuture.get()` threw an exception, in which case the `finally` block will be trigerred before all threads complete?
>
> I think the solution you have proposed works fine, but the `Future#cancel` methodology should work too and is slightly cleaner, but if you can't get it to work no worries, don't spend too much time on it
>
> Marta Kuczora wrote:
> Yep, I checked it and for these "leftover" tasks, the cancel returned true. There were tasks for which the cancel returned false, but those were ok, because the get just waited for them.
>
> No, I didn't get exception during the second partFuture.get(). Also when I logged what happens with the tasks, all tasks appeared in this second get loop. For the ones which were canceled successfully (even though they are running), just showed that they were canceled.
>
> I agree that it would be a cleaner solution and I spent quite some time with trying to make it work, but I could make this use case work.
Ok sounds good. The current implementation looks fine then. Let's just stick to this then. Overall, LGTM pending the refactoring.
- Sahil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65716/#review197829
-----------------------------------------------------------
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
>
>
Re: Review Request 65716: HIVE-18696: The partition folders might not
get
cleaned up properly in the HiveMetaStore.add_partitions_core method if an
exception occurs
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
> On Feb. 21, 2018, 3:32 p.m., Sahil Takiar wrote:
> > Would be good to know which Hive queries invoke this method.
Thanks a lot Sahil for the review. I will check where these methods are used and come back to you with the answer a bit later.
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65716/#review197829
-----------------------------------------------------------
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
>
>
Re: Review Request 65716: HIVE-18696: The partition folders might not
get
cleaned up properly in the HiveMetaStore.add_partitions_core method if an
exception occurs
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
> On Feb. 21, 2018, 3:32 p.m., Sahil Takiar wrote:
> > Would be good to know which Hive queries invoke this method.
>
> Marta Kuczora wrote:
> Thanks a lot Sahil for the review. I will check where these methods are used and come back to you with the answer a bit later.
I checked how these methods are used.
The 'add_partition_core' method is used when adding multiple partitions to the table. A simple query, like the following, can trigger it.
ALTER TABLE bubu ADD PARTITION (year='2017',month='march') PARTITION (year='2017',month='april') PARTITION (year='2018',month='march') PARTITION (year='2018',month='may') PARTITION (year='2017',month='march', day="3");
It is also used by the DDLTask.createPartitionsInBatches method which is used by the 'msck repair' command.
I didn't find much about the 'add_partitions_pspec_core' method. I only found that it is used by the HCatClientHMSImpl.addPartitionSpec method, but I don't know if the HCatClientHMSImpl is used or how it is used.
> On Feb. 21, 2018, 3:32 p.m., Sahil Takiar wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Line 3032 (original), 3065 (patched)
> > <https://reviews.apache.org/r/65716/diff/1/?file=1962881#file1962881line3072>
> >
> > this code looks very similar to the block above. I know its was never the intention of this JIRA to do any re-factoring, but how difficult would it be to move all this code into a common method so that we don't have to fix the bug in two places? not a blocking issue though
>
> Marta Kuczora wrote:
> Yeah, I absolutely agree. This code duplication annoys me as well, just I wasn't sure that it is acceptable doing the refactoring in the scope of this Jira. But it is not so difficult, so I will upload a patch where I moved the common parts to a separate method and we can decide if it is ok like that or rather do it in a different Jira.
>
> Marta Kuczora wrote:
> I checked how this could be refactored and there are some differences between the methods which makes it not that straightforward. It is not that difficult and basically I have the patch, but I would do it in the scope of an other Jira, so we can discuss some details there. Would this be ok for you Sahil?
Created a follow-up Jira for the refactoring
https://issues.apache.org/jira/browse/HIVE-19046
- Marta
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65716/#review197829
-----------------------------------------------------------
On March 8, 2018, 4:52 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65716/
> -----------------------------------------------------------
>
> (Updated March 8, 2018, 4:52 p.m.)
>
>
> Review request for hive, Alexander Kolbasov, 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 662de9a
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java 4d9cb1b
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java 1122057
>
>
> Diff: https://reviews.apache.org/r/65716/diff/3/
>
>
> Testing
> -------
>
> Added some new tests cases to the TestAddPartitions and TestAddPartitionsFromPartSpec tests.
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 65716: HIVE-18696: The partition folders might not
get
cleaned up properly in the HiveMetaStore.add_partitions_core method if an
exception occurs
Posted by Sahil Takiar <ta...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65716/#review197829
-----------------------------------------------------------
Would be good to know which Hive queries invoke this method.
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Line 3032 (original), 3065 (patched)
<https://reviews.apache.org/r/65716/#comment278153>
this code looks very similar to the block above. I know its was never the intention of this JIRA to do any re-factoring, but how difficult would it be to move all this code into a common method so that we don't have to fix the bug in two places? not a blocking issue though
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 3149-3150 (patched)
<https://reviews.apache.org/r/65716/#comment278150>
curious what behavior you were seeing, wondering why cancelling or interrupting the `Future`s doesn't work
- Sahil Takiar
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
>
>