Re: Review Request 65217: Create tests to cover alterPartition and renamePartition methods

2018-01-25 Thread Adam Szita via Review Board 

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

2018-01-25 Thread Marta Kuczora via Review Board 

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

2018-01-23 Thread Adam Szita via Review Board 


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

2018-01-23 Thread Adam Szita via Review Board 

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

2018-01-22 Thread Marta Kuczora via Review Board 

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


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)


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)


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)


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)


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

2018-01-19 Thread Adam Szita via Review Board 


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

2018-01-19 Thread Adam Szita via Review Board 

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

2018-01-19 Thread Peter Vary via Review Board 

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


nit: We should import everything which is needed



standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
Lines 122 (patched)


nit: Why static?



standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java
Lines 150 (patched)


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)


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)


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)


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

Review Request 65217: Create tests to cover alterPartition and renamePartition methods

2018-01-18 Thread Adam Szita via Review Board 

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