Re: Review Request 65213: HIVE-18479: Create tests to cover methods for dropping Partitions

[Peter Vary via Review Board]

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


Ship it!




Ship It!

- Peter Vary


On Jan. 24, 2018, 4:37 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65213/
> ---
> 
> (Updated Jan. 24, 2018, 4:37 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18479
> https://issues.apache.org/jira/browse/HIVE-18479
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - boolean dropPartition(String, String, List, boolean)
> - boolean dropPartition(String, String, List, PartitionDropOptions)
> - boolean dropPartition(String, String, String, 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/TestDropPartitions.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
>  ed071f8 
> 
> 
> Diff: https://reviews.apache.org/r/65213/diff/5/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65213: HIVE-18479: Create tests to cover methods for dropping Partitions

[Peter Vary via Review Board]

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


Ship it!




Ship It!

- Peter Vary


On Jan. 24, 2018, 4:37 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65213/
> ---
> 
> (Updated Jan. 24, 2018, 4:37 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18479
> https://issues.apache.org/jira/browse/HIVE-18479
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - boolean dropPartition(String, String, List, boolean)
> - boolean dropPartition(String, String, List, PartitionDropOptions)
> - boolean dropPartition(String, String, String, 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/TestDropPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65213/diff/4/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65213: HIVE-18479: Create tests to cover methods for dropping Partitions

[Marta Kuczora via Review Board]

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

(Updated Jan. 24, 2018, 4:37 p.m.)


Review request for hive, Peter Vary and Adam Szita.


Changes
---

Fixed some stylecheck issues.


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


Repository: hive-git


Description
---

The following methods of IMetaStoreClient are covered by this test.
- boolean dropPartition(String, String, List, boolean)
- boolean dropPartition(String, String, List, PartitionDropOptions)
- boolean dropPartition(String, String, String, 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/TestDropPartitions.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/65213/diff/4/

Changes: https://reviews.apache.org/r/65213/diff/3-4/


Testing
---

Run the tests


Thanks,

Marta Kuczora

Re: Review Request 65213: HIVE-18479: Create tests to cover methods for dropping Partitions

[Marta Kuczora via Review Board]

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

(Updated Jan. 23, 2018, 1:57 p.m.)


Review request for hive, Peter Vary and Adam Szita.


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


Repository: hive-git


Description
---

The following methods of IMetaStoreClient are covered by this test.
- boolean dropPartition(String, String, List, boolean)
- boolean dropPartition(String, String, List, PartitionDropOptions)
- boolean dropPartition(String, String, String, 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/TestDropPartitions.java
 PRE-CREATION 


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

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


Testing
---

Run the tests


Thanks,

Marta Kuczora

Re: Review Request 65213: HIVE-18479: Create tests to cover methods for dropping Partitions

[Marta Kuczora via Review Board]

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

(Updated Jan. 22, 2018, 12:38 p.m.)


Review request for hive, Peter Vary and Adam Szita.


Changes
---

Fixed the review findings.


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


Repository: hive-git


Description
---

The following methods of IMetaStoreClient are covered by this test.
- boolean dropPartition(String, String, List, boolean)
- boolean dropPartition(String, String, List, PartitionDropOptions)
- boolean dropPartition(String, String, String, 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/TestDropPartitions.java
 PRE-CREATION 


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

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


Testing
---

Run the tests


Thanks,

Marta Kuczora

Re: Review Request 65213: HIVE-18479: Create tests to cover methods for dropping Partitions

[Marta Kuczora via Review Board]

> On Jan. 19, 2018, 11:50 a.m., Peter Vary wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
> > Lines 341 (patched)
> > 
> >
> > Question: Does this behave the same way for external tables too?

Thanks a lot Peter for the review.

That's a good point. :) No, with external tables, the data won't be deleted.
Added an additional test case where deleteData = purge = true with an external 
table and the test checks if the partitions is dropped but the location is not 
deleted.


- Marta


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


On Jan. 22, 2018, 12:38 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65213/
> ---
> 
> (Updated Jan. 22, 2018, 12:38 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18479
> https://issues.apache.org/jira/browse/HIVE-18479
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - boolean dropPartition(String, String, List, boolean)
> - boolean dropPartition(String, String, List, PartitionDropOptions)
> - boolean dropPartition(String, String, String, 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/TestDropPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65213/diff/2/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65213: HIVE-18479: Create tests to cover methods for dropping Partitions

[Marta Kuczora via Review Board]

> On Jan. 19, 2018, 1:02 p.m., Adam Szita wrote:
> > Overall looks good, I just have small observations.
> > 
> > One more nitpick: some helper methods could be static e.g. 
> > getYearAndMonthCols

Thanks a lot Adam for the review.

Good point, changed the getYearAndMonthPartCols and getYearPartCol helper 
methods to be static.


> On Jan. 19, 2018, 1:02 p.m., Adam Szita wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
> > Lines 64 (patched)
> > 
> >
> > Is there any reason to use 10? Supplying -1 would return all partitions 
> > which I think was the original intent in the test cases below (in 
> > listPartitions calls).

No specific reason, I just missed this. This is a good point, thanks for it.


> On Jan. 19, 2018, 1:02 p.m., Adam Szita wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
> > Lines 165 (patched)
> > 
> >
> > createTable is called here twice because it's also in @Before. Does 
> > this mean it overrides the existing table with the supplied tableParams?

In this test case I wanted to create a different table with the 
"auto.purge=true" table parameter. This table has the name "purge_test". In the 
@Before method, I create the default table with the name 
"test_drop_part_table". So there will be two tables in this test case.


> On Jan. 19, 2018, 1:02 p.m., Adam Szita wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
> > Lines 539-553 (patched)
> > 
> >
> > Guava lib has a handy method for these use cases:
> > 
> >  ArrayList com.google.common.collect.Lists.newArrayList(E... 
> > elements)
> > 
> > ...which is doing exactly the same + optimising on initial list size.

That's an excellent point, thanks a lot for the hint. I didn't know this lib 
before.


- Marta


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


On Jan. 18, 2018, 1:11 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65213/
> ---
> 
> (Updated Jan. 18, 2018, 1:11 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18479
> https://issues.apache.org/jira/browse/HIVE-18479
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - boolean dropPartition(String, String, List, boolean)
> - boolean dropPartition(String, String, List, PartitionDropOptions)
> - boolean dropPartition(String, String, String, 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/TestDropPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65213/diff/1/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65213: HIVE-18479: Create tests to cover methods for dropping Partitions

[Adam Szita via Review Board]

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



Overall looks good, I just have small observations.

One more nitpick: some helper methods could be static e.g. getYearAndMonthCols


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


Is there any reason to use 10? Supplying -1 would return all partitions 
which I think was the original intent in the test cases below (in 
listPartitions calls).



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


createTable is called here twice because it's also in @Before. Does this 
mean it overrides the existing table with the supplied tableParams?



standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDropPartitions.java
Lines 539-553 (patched)


Guava lib has a handy method for these use cases:

 ArrayList com.google.common.collect.Lists.newArrayList(E... elements)

...which is doing exactly the same + optimising on initial list size.


- Adam Szita


On Jan. 18, 2018, 1:11 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65213/
> ---
> 
> (Updated Jan. 18, 2018, 1:11 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18479
> https://issues.apache.org/jira/browse/HIVE-18479
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - boolean dropPartition(String, String, List, boolean)
> - boolean dropPartition(String, String, List, PartitionDropOptions)
> - boolean dropPartition(String, String, String, 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/TestDropPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65213/diff/1/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Re: Review Request 65213: HIVE-18479: Create tests to cover methods for dropping Partitions

[Peter Vary via Review Board]

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



Thanks Marta for the patch.
Just one question.

Peter


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


Question: Does this behave the same way for external tables too?


- Peter Vary


On Jan. 18, 2018, 1:11 p.m., Marta Kuczora wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65213/
> ---
> 
> (Updated Jan. 18, 2018, 1:11 p.m.)
> 
> 
> Review request for hive, Peter Vary and Adam Szita.
> 
> 
> Bugs: HIVE-18479
> https://issues.apache.org/jira/browse/HIVE-18479
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> The following methods of IMetaStoreClient are covered by this test.
> - boolean dropPartition(String, String, List, boolean)
> - boolean dropPartition(String, String, List, PartitionDropOptions)
> - boolean dropPartition(String, String, String, 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/TestDropPartitions.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65213/diff/1/
> 
> 
> Testing
> ---
> 
> Run the tests
> 
> 
> Thanks,
> 
> Marta Kuczora
> 
>

Review Request 65213: HIVE-18479: Create tests to cover methods for dropping Partitions

[Marta Kuczora via Review Board]

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

Review request for hive, Peter Vary and Adam Szita.


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


Repository: hive-git


Description
---

The following methods of IMetaStoreClient are covered by this test.
- boolean dropPartition(String, String, List, boolean)
- boolean dropPartition(String, String, List, PartitionDropOptions)
- boolean dropPartition(String, String, String, 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/TestDropPartitions.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/65213/diff/1/


Testing
---

Run the tests


Thanks,

Marta Kuczora