You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Adam Szita via Review Board <no...@reviews.apache.org> on 2018/01/18 14:26:40 UTC
Review Request 65217: Create tests to cover alterPartition and
renamePartition methods
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65217/
-----------------------------------------------------------
Review request for hive, Marta Kuczora and Peter Vary.
Bugs: HIVE-18468
https://issues.apache.org/jira/browse/HIVE-18468
Repository: hive-git
Description
-------
Adding IMetastoreClient API tests that cover alterPartition ande renamePartition methods
Diffs
-----
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65217/diff/1/
Testing
-------
Thanks,
Adam Szita
Re: Review Request 65217: Create tests to cover alterPartition and
renamePartition 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/65217/#review196222
-----------------------------------------------------------
Ship it!
Ship It!
- Marta Kuczora
On Jan. 23, 2018, 1:38 p.m., Adam Szita wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65217/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2018, 1:38 p.m.)
>
>
> Review request for hive, Marta Kuczora and Peter Vary.
>
>
> Bugs: HIVE-18468
> https://issues.apache.org/jira/browse/HIVE-18468
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Adding IMetastoreClient API tests that cover alterPartition ande renamePartition methods
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65217/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Adam Szita
>
>
Re: Review Request 65217: Create tests to cover alterPartition and
renamePartition methods
Posted by Adam Szita via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65217/
-----------------------------------------------------------
(Updated Jan. 25, 2018, 1:52 p.m.)
Review request for hive, Marta Kuczora and Peter Vary.
Changes
-------
Checkstyle fixes
Bugs: HIVE-18468
https://issues.apache.org/jira/browse/HIVE-18468
Repository: hive-git
Description
-------
Adding IMetastoreClient API tests that cover alterPartition ande renamePartition methods
Diffs (updated)
-----
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65217/diff/4/
Changes: https://reviews.apache.org/r/65217/diff/3-4/
Testing
-------
Thanks,
Adam Szita
Re: Review Request 65217: Create tests to cover alterPartition and
renamePartition methods
Posted by Adam Szita via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65217/
-----------------------------------------------------------
(Updated Jan. 23, 2018, 1:38 p.m.)
Review request for hive, Marta Kuczora and Peter Vary.
Changes
-------
Added additional cases as per Marta's comments
Bugs: HIVE-18468
https://issues.apache.org/jira/browse/HIVE-18468
Repository: hive-git
Description
-------
Adding IMetastoreClient API tests that cover alterPartition ande renamePartition methods
Diffs (updated)
-----
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65217/diff/3/
Changes: https://reviews.apache.org/r/65217/diff/2-3/
Testing
-------
Thanks,
Adam Szita
Re: Review Request 65217: Create tests to cover alterPartition and
renamePartition methods
Posted by Adam Szita via Review Board <no...@reviews.apache.org>.
> On Jan. 22, 2018, 3:21 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 223 (patched)
> > <https://reviews.apache.org/r/65217/diff/2/?file=1942597#file1942597line223>
> >
> > When altering a partition, is it possible to modify its StorageDescriptor and location?
> > If so, what happens if we try to modify the sd or location to null or empty?
> > Is it possible to change the columns in the partition's storage descriptor?
SD can be changed completely, I change location by hand and I also added a change for col list now, thanks!
> On Jan. 22, 2018, 3:21 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 497 (patched)
> > <https://reviews.apache.org/r/65217/diff/2/?file=1942597#file1942597line497>
> >
> > When tyring to alter two partitions, but an exception occurs during altering one of them, what happens with the other? All modification will be rolled back correctly?
Altered my test case to check this, it is rolled back correctly
> On Jan. 22, 2018, 3:21 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 769 (patched)
> > <https://reviews.apache.org/r/65217/diff/2/?file=1942597#file1942597line769>
> >
> > What happens if the newPart partition has different db/table than the ones set in the method parameter?
Added test case for this
> On Jan. 22, 2018, 3:21 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 770 (patched)
> > <https://reviews.apache.org/r/65217/diff/2/?file=1942597#file1942597line770>
> >
> > What happens if the db/table is null in the newPart partition?
Added test case for this
> On Jan. 22, 2018, 3:21 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 771 (patched)
> > <https://reviews.apache.org/r/65217/diff/2/?file=1942597#file1942597line771>
> >
> > Is it possible to change any other attribute of a partition besides the value with the rename method?
Added test case for this
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65217/#review195910
-----------------------------------------------------------
On Jan. 23, 2018, 1:38 p.m., Adam Szita wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65217/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2018, 1:38 p.m.)
>
>
> Review request for hive, Marta Kuczora and Peter Vary.
>
>
> Bugs: HIVE-18468
> https://issues.apache.org/jira/browse/HIVE-18468
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Adding IMetastoreClient API tests that cover alterPartition ande renamePartition methods
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65217/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Adam Szita
>
>
Re: Review Request 65217: Create tests to cover alterPartition and
renamePartition 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/65217/#review195910
-----------------------------------------------------------
Thanks a lot Adam for the patch.
It looks good to me, I just have few questions.
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
Lines 223 (patched)
<https://reviews.apache.org/r/65217/#comment275266>
When altering a partition, is it possible to modify its StorageDescriptor and location?
If so, what happens if we try to modify the sd or location to null or empty?
Is it possible to change the columns in the partition's storage descriptor?
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
Lines 497 (patched)
<https://reviews.apache.org/r/65217/#comment275267>
When tyring to alter two partitions, but an exception occurs during altering one of them, what happens with the other? All modification will be rolled back correctly?
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
Lines 769 (patched)
<https://reviews.apache.org/r/65217/#comment275268>
What happens if the newPart partition has different db/table than the ones set in the method parameter?
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
Lines 770 (patched)
<https://reviews.apache.org/r/65217/#comment275269>
What happens if the db/table is null in the newPart partition?
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
Lines 771 (patched)
<https://reviews.apache.org/r/65217/#comment275270>
Is it possible to change any other attribute of a partition besides the value with the rename method?
- Marta Kuczora
On Jan. 19, 2018, 2:29 p.m., Adam Szita wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65217/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2018, 2:29 p.m.)
>
>
> Review request for hive, Marta Kuczora and Peter Vary.
>
>
> Bugs: HIVE-18468
> https://issues.apache.org/jira/browse/HIVE-18468
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Adding IMetastoreClient API tests that cover alterPartition ande renamePartition methods
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65217/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Adam Szita
>
>
Re: Review Request 65217: Create tests to cover alterPartition and
renamePartition methods
Posted by Adam Szita via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65217/
-----------------------------------------------------------
(Updated Jan. 19, 2018, 2:29 p.m.)
Review request for hive, Marta Kuczora and Peter Vary.
Changes
-------
Fixed smaller issues
Bugs: HIVE-18468
https://issues.apache.org/jira/browse/HIVE-18468
Repository: hive-git
Description
-------
Adding IMetastoreClient API tests that cover alterPartition ande renamePartition methods
Diffs (updated)
-----
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java PRE-CREATION
Diff: https://reviews.apache.org/r/65217/diff/2/
Changes: https://reviews.apache.org/r/65217/diff/1-2/
Testing
-------
Thanks,
Adam Szita
Re: Review Request 65217: Create tests to cover alterPartition and
renamePartition methods
Posted by Adam Szita via Review Board <no...@reviews.apache.org>.
> On Jan. 19, 2018, 11:41 a.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 122 (patched)
> > <https://reviews.apache.org/r/65217/diff/1/?file=1942096#file1942096line122>
> >
> > nit: Why static?
I like my helper methods static :)
> On Jan. 19, 2018, 11:41 a.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 150 (patched)
> > <https://reviews.apache.org/r/65217/diff/1/?file=1942096#file1942096line150>
> >
> > nit: Can we stick to one notation? I do not like _ in function names :)
fixed
> On Jan. 19, 2018, 11:41 a.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
> > Lines 450 (patched)
> > <https://reviews.apache.org/r/65217/diff/1/?file=1942096#file1942096line450>
> >
> > Question: Maybe in setup?
Some tests depend on not having a table in HMS.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65217/#review195795
-----------------------------------------------------------
On Jan. 19, 2018, 2:29 p.m., Adam Szita wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65217/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2018, 2:29 p.m.)
>
>
> Review request for hive, Marta Kuczora and Peter Vary.
>
>
> Bugs: HIVE-18468
> https://issues.apache.org/jira/browse/HIVE-18468
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Adding IMetastoreClient API tests that cover alterPartition ande renamePartition methods
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65217/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Adam Szita
>
>
Re: Review Request 65217: Create tests to cover alterPartition and
renamePartition methods
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/65217/#review195795
-----------------------------------------------------------
Thanks for the patch Adam.
Questions, and some minor comments.
Thanks,
Peter
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
Lines 52 (patched)
<https://reviews.apache.org/r/65217/#comment275113>
nit: We should import everything which is needed
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
Lines 122 (patched)
<https://reviews.apache.org/r/65217/#comment275112>
nit: Why static?
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
Lines 150 (patched)
<https://reviews.apache.org/r/65217/#comment275111>
nit: Can we stick to one notation? I do not like _ in function names :)
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
Lines 181 (patched)
<https://reviews.apache.org/r/65217/#comment275115>
These are the only parameters which can be changed?
If so should not we check the other paramteres are not changed in assertPartitionChanged?
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
Lines 298 (patched)
<https://reviews.apache.org/r/65217/#comment275132>
nit: We might want to add TODO comments to places where we think we want to change
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
Lines 450 (patched)
<https://reviews.apache.org/r/65217/#comment275133>
Question: Maybe in setup?
- Peter Vary
On Jan. 18, 2018, 2:26 p.m., Adam Szita wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65217/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2018, 2:26 p.m.)
>
>
> Review request for hive, Marta Kuczora and Peter Vary.
>
>
> Bugs: HIVE-18468
> https://issues.apache.org/jira/browse/HIVE-18468
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Adding IMetastoreClient API tests that cover alterPartition ande renamePartition methods
>
>
> Diffs
> -----
>
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65217/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Adam Szita
>
>