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/05/23 16:24:19 UTC
Re: Review Request 66667: HIVE-19046: Refactor the common parts of the
HiveMetastore add_partition_core and add_partitions_pspec_core methods
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66667/
-----------------------------------------------------------
(Updated May 23, 2018, 4:24 p.m.)
Review request for hive, Peter Vary, Sahil Takiar, and Adam Szita.
Changes
-------
Address review finding.
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 (updated)
-----
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 92d2e3f
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java 88064d9
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java debcd0e
Diff: https://reviews.apache.org/r/66667/diff/3/
Changes: https://reviews.apache.org/r/66667/diff/2-3/
Testing
-------
Thanks,
Marta Kuczora
Re: Review Request 66667: HIVE-19046: Refactor the common parts of the
HiveMetastore add_partition_core and add_partitions_pspec_core methods
Posted by Marta Kuczora via Review Board <no...@reviews.apache.org>.
> 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
>
>
Re: Review Request 66667: HIVE-19046: Refactor the common parts of the
HiveMetastore add_partition_core and add_partitions_pspec_core methods
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66667/#review203099
-----------------------------------------------------------
Looks good, a few nits below.
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Line 3227 (original), 3322 (patched)
<https://reviews.apache.org/r/66667/#comment285171>
Is it possible to do it once in constructor instead? I suspect that this is a no-trivial operation.
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 3253 (patched)
<https://reviews.apache.org/r/66667/#comment286933>
Can you clarify that "clean up" means removing associated directory.
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 3268 (patched)
<https://reviews.apache.org/r/66667/#comment286934>
Please add a Javadoc here explaining what is checked by validation. Also it isn't obvious that validation has side effects (updating partsToAdd)
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Line 3247 (original), 3343 (patched)
<https://reviews.apache.org/r/66667/#comment286935>
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.
- Alexander Kolbasov
On June 1, 2018, 12:31 p.m., Marta Kuczora wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66667/
> -----------------------------------------------------------
>
> (Updated June 1, 2018, 12:31 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 d8b8414
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java 88064d9
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java debcd0e
>
>
> Diff: https://reviews.apache.org/r/66667/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Marta Kuczora
>
>
Re: Review Request 66667: HIVE-19046: Refactor the common parts of the
HiveMetastore add_partition_core and add_partitions_pspec_core methods
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/66667/
-----------------------------------------------------------
(Updated June 25, 2018, 12:01 p.m.)
Review request for hive, Peter Vary, Sahil Takiar, and Adam Szita.
Changes
-------
Rebased the patch
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 (updated)
-----
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java e9d7e7c
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/7/
Changes: https://reviews.apache.org/r/66667/diff/6-7/
Testing
-------
Thanks,
Marta Kuczora
Re: Review Request 66667: HIVE-19046: Refactor the common parts of the
HiveMetastore add_partition_core and add_partitions_pspec_core methods
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/66667/
-----------------------------------------------------------
(Updated June 11, 2018, 11:27 a.m.)
Review request for hive, Peter Vary, Sahil Takiar, and Adam Szita.
Changes
-------
Address review findings.
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/66667/diff/5-6/
Testing
-------
Thanks,
Marta Kuczora
Re: Review Request 66667: HIVE-19046: Refactor the common parts of the
HiveMetastore add_partition_core and add_partitions_pspec_core methods
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/66667/
-----------------------------------------------------------
(Updated June 1, 2018, 12:31 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 (updated)
-----
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java d8b8414
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java 88064d9
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java debcd0e
Diff: https://reviews.apache.org/r/66667/diff/5/
Changes: https://reviews.apache.org/r/66667/diff/4-5/
Testing
-------
Thanks,
Marta Kuczora
Re: Review Request 66667: HIVE-19046: Refactor the common parts of the
HiveMetastore add_partition_core and add_partitions_pspec_core methods
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/66667/
-----------------------------------------------------------
(Updated May 29, 2018, 4:24 p.m.)
Review request for hive, Peter Vary, Sahil Takiar, and Adam Szita.
Changes
-------
Rebased the patch.
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 (updated)
-----
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java c1d25db
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java 88064d9
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitionsFromPartSpec.java debcd0e
Diff: https://reviews.apache.org/r/66667/diff/4/
Changes: https://reviews.apache.org/r/66667/diff/3-4/
Testing
-------
Thanks,
Marta Kuczora