Re: Review Request 65219: HIVE-18486: Create tests to cover methods for adding Partitions

2018-01-27 Thread Marta Kuczora via Review Board 

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

(Updated Jan. 27, 2018, 10:01 a.m.)


Review request for hive, Peter Vary and Adam Szita.


Changes
---

Fixed the failing TestAddPartitions.testAddPartitionsOneInvalid test. This test 
was failing due to a concurrency issue. There is already a TODO about this 
issue, so now just commented out the problematic check and investigate this 
issue in an other Jira.


Bugs: HIVE-18486
https://issues.apache.org/jira/browse/HIVE-18486


Repository: hive-git


Description
---

The following methods of IMetaStoreClient are covered by this test.
- Partition add_partition(Partition)
- int add_partitions(List)
- List add_partitions(List, boolean, boolean)


The test covers not just the happy pathes, but the edge cases as well.


Diffs (updated)
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/65219/diff/3/

Changes: https://reviews.apache.org/r/65219/diff/2-3/


Testing
---

Run the tests


Thanks,

Marta Kuczora

Re: Review Request 65219: HIVE-18486: Create tests to cover methods for adding Partitions

2018-01-25 Thread Peter Vary via Review Board 

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


Ship it!




Ship It!

- Peter Vary


On Jan. 25, 2018, 2:40 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65219/
> ---
> 
> (Updated Jan. 25, 2018, 2:40 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18486
> https://issues.apache.org/jira/browse/HIVE-18486
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - Partition add_partition(Partition)
> - int add_partitions(List)
> - List add_partitions(List, boolean, boolean)
> 
> 
> The test covers not just the happy pathes, but the edge cases as well.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65219/diff/2/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65219: HIVE-18486: Create tests to cover methods for adding Partitions

2018-01-25 Thread Marta Kuczora via Review Board 

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

(Updated Jan. 25, 2018, 2:40 p.m.)


Review request for hive, Peter Vary and Adam Szita.


Changes
---

Addressed the review findings.
- Added tests for creating partitions with default attribute values.
- Added tests for creating partitions without columns and with invalid or 
incomplete colum infos
- Added tests for creating partitions without serde info
- Added tests for creating partitions for external table with and without 
location being set
- Made some helper methods static
- Used the List.newArrayList method where it is needed
- Changed the value of the MAX variable to -1
- Removed the creation time checks where it was not necessary
- Fixed some checkstyle issues


Bugs: HIVE-18486
https://issues.apache.org/jira/browse/HIVE-18486


Repository: hive-git


Description
---

The following methods of IMetaStoreClient are covered by this test.
- Partition add_partition(Partition)
- int add_partitions(List)
- List add_partitions(List, boolean, boolean)


The test covers not just the happy pathes, but the edge cases as well.


Diffs (updated)
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/65219/diff/2/

Changes: https://reviews.apache.org/r/65219/diff/1-2/


Testing
---

Run the tests


Thanks,

Marta Kuczora

Re: Review Request 65219: HIVE-18486: Create tests to cover methods for adding Partitions

2018-01-25 Thread Marta Kuczora via Review Board 


> On Jan. 19, 2018, 4:36 p.m., Adam Szita wrote:
> > Looks good! I added small issues to fix, similarly with the dropPartitions 
> > change: some helper methods could be static (the ones that don't depend on 
> > HMS client certainly) and some could use Lists.newArrayList

Thanks a lot Adam for the review.
I made some helper methods static and also used the Lists.newArrayList where is 
was needed.


> On Jan. 19, 2018, 4:36 p.m., Adam Szita wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Lines 68 (patched)
> > 
> >
> > Again this may be -1 instead, when MAX as a value is not used for any 
> > other reason than returning all partitions

No reason, just missed it. Thanks for the hint, fixed it.


> On Jan. 19, 2018, 4:36 p.m., Adam Szita wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Lines 672 (patched)
> > 
> >
> > This is found multiple times in the code, should we create a method for 
> > it?

I refactored a bit handling the creation time, so these duplications are 
eliminated.


- Marta


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


On Jan. 18, 2018, 4:58 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65219/
> ---
> 
> (Updated Jan. 18, 2018, 4:58 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18486
> https://issues.apache.org/jira/browse/HIVE-18486
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - Partition add_partition(Partition)
> - int add_partitions(List)
> - List add_partitions(List, boolean, boolean)
> 
> 
> The test covers not just the happy pathes, but the edge cases as well.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65219/diff/1/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65219: HIVE-18486: Create tests to cover methods for adding Partitions

2018-01-25 Thread Marta Kuczora via Review Board 


> On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote:
> > Thanks Marta for the patch.
> > Some questions below.
> > Also I think it would be nice to have test for the default values when 
> > adding a partition.
> > 
> > Thanks,
> > Peter

Thanks a lot Peter for the review.
That's a good idea, added tests for creating partitions with default values.


> On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Lines 236 (patched)
> > 
> >
> > There are some interesting validation in the SD. Maybe we can add test 
> > for it here too.

Good point, thanks. Created tests for some use cases.


> On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Lines 282 (patched)
> > 
> >
> > Is it worth to create a test for an External Table too?

Creating partitions for an external table doesn't really differ from creating 
it for a managed table. But I agree it worth to have a test case for that too. 
I added it.


> On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Lines 700 (patched)
> > 
> >
> > Like above - validation inside the SD might be important as well

Created some test cases for sd attribute validations.


> On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Lines 749 (patched)
> > 
> >
> > Maybe for external table too. And maybe check the locations

Added tests for external table with and without setting the location.


> On Jan. 19, 2018, 1:56 p.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
> > Lines 1117 (patched)
> > 
> >
> > I do not understand this check

If the 'metastore.partition.inherit.table.properties' property is set in the 
metastore config, the partition inherits the listed table parameters.
This property is not set in this test, therefore the partition shouldn't 
inherit the table parameters. That is what this check is for.
But it is true that this is not obvious, so I added a comment to this check.


- Marta


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


On Jan. 18, 2018, 4:58 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65219/
> ---
> 
> (Updated Jan. 18, 2018, 4:58 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18486
> https://issues.apache.org/jira/browse/HIVE-18486
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - Partition add_partition(Partition)
> - int add_partitions(List)
> - List add_partitions(List, boolean, boolean)
> 
> 
> The test covers not just the happy pathes, but the edge cases as well.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65219/diff/1/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65219: HIVE-18486: Create tests to cover methods for adding Partitions

2018-01-19 Thread Adam Szita via Review Board 

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



Looks good! I added small issues to fix, similarly with the dropPartitions 
change: some helper methods could be static (the ones that don't depend on HMS 
client certainly) and some could use Lists.newArrayList


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


Again this may be -1 instead, when MAX as a value is not used for any other 
reason than returning all partitions



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


This is found multiple times in the code, should we create a method for it?


- Adam Szita


On Jan. 18, 2018, 4:58 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65219/
> ---
> 
> (Updated Jan. 18, 2018, 4:58 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18486
> https://issues.apache.org/jira/browse/HIVE-18486
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - Partition add_partition(Partition)
> - int add_partitions(List)
> - List add_partitions(List, boolean, boolean)
> 
> 
> The test covers not just the happy pathes, but the edge cases as well.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65219/diff/1/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65219: HIVE-18486: Create tests to cover methods for adding Partitions

2018-01-19 Thread Peter Vary via Review Board 

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



Thanks Marta for the patch.
Some questions below.
Also I think it would be nice to have test for the default values when adding a 
partition.

Thanks,
Peter


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


There are some interesting validation in the SD. Maybe we can add test for 
it here too.



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


Is it worth to create a test for an External Table too?



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


Like above - validation inside the SD might be important as well



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


Maybe for external table too. And maybe check the locations



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


I do not understand this check


- Peter Vary


On Jan. 18, 2018, 4:58 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65219/
> ---
> 
> (Updated Jan. 18, 2018, 4:58 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18486
> https://issues.apache.org/jira/browse/HIVE-18486
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - Partition add_partition(Partition)
> - int add_partitions(List)
> - List add_partitions(List, boolean, boolean)
> 
> 
> The test covers not just the happy pathes, but the edge cases as well.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65219/diff/1/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>