Re: Review Request 65018: HIVE-18372 Create testing infra to test different HMS instances

[Peter Vary via Review Board]

> On Jan. 10, 2018, 6:35 p.m., Vihang Karajgaonkar wrote:
> > Overall looks good. Thanks for taking this up. I suggested some small 
> > improvements to the tests below.

Thanks Adam and Vihang for the review!


> On Jan. 10, 2018, 6:35 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
> > Lines 350 (patched)
> > 
> >
> > I think we should catch the exception and assert the exception type is 
> > InvalidOperationException so that we catch errors like if someone changes 
> > the thrown exception in the future.
> 
> Peter Vary wrote:
> Shall I separate the test case into two? With/Without cascade? There is a 
> little extra stuff there creating the table/function/index in two tests which 
> I wanted to avoid with one test case. That is why I decied to keep the 
> cascade tests in one test case and used try-catch here to check the exception 
> without the cascade option, and then proceed with the test and drop the 
> database with the cascade option.
> 
> What do you think?
> 
> Adam Szita wrote:
> I'd vote for separation. One test case for cascade and one for without 
> cascade. The latter should have an @Test(expected = ..) annotation IMHO

Since this seems like a consensus, spearated the tests :)


- Peter


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


On Jan. 12, 2018, 11:58 a.m., Peter Vary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65018/
> ---
> 
> (Updated Jan. 12, 2018, 11:58 a.m.)
> 
> 
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-18372
> https://issues.apache.org/jira/browse/HIVE-18372
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Created:
> - AbstractMetastore class - to privide an interface for different metastore 
> implementation (start/stop/warehouse path methods)
> -- Implementation for Embedded/Remote/Cluster metastores
> - MiniHMS with builder - to create hms instances for test
> - MetaStoreFactory - to create the parameter list for parametrized test
> - TestDatabases - test for database related metastore functions to showcase 
> the infrastructure
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/MetaStoreFactoryForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/ClusterMetaStoreForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/EmbeddedMetaStoreForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/MiniHMS.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/RemoteMetaStoreForTests.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65018/diff/5/
> 
> 
> Testing
> ---
> 
> Run the new tests
> 
> 
> Thanks,
> 
> Peter Vary
> 
>

Re: Review Request 65018: HIVE-18372 Create testing infra to test different HMS instances

[Peter Vary via Review Board]

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

(Updated Jan. 12, 2018, 11:58 a.m.)


Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
Karajgaonkar.


Changes
---

Addressed Vihang's and Adam's comments


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


Repository: hive-git


Description
---

Created:
- AbstractMetastore class - to privide an interface for different metastore 
implementation (start/stop/warehouse path methods)
-- Implementation for Embedded/Remote/Cluster metastores
- MiniHMS with builder - to create hms instances for test
- MetaStoreFactory - to create the parameter list for parametrized test
- TestDatabases - test for database related metastore functions to showcase the 
infrastructure


Diffs (updated)
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/MetaStoreFactoryForTests.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/ClusterMetaStoreForTests.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/EmbeddedMetaStoreForTests.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/MiniHMS.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/RemoteMetaStoreForTests.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/65018/diff/5/

Changes: https://reviews.apache.org/r/65018/diff/4-5/


Testing
---

Run the new tests


Thanks,

Peter Vary

Re: Review Request 65018: HIVE-18372 Create testing infra to test different HMS instances

[Adam Szita via Review Board]

> On Jan. 10, 2018, 6:35 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
> > Lines 350 (patched)
> > 
> >
> > I think we should catch the exception and assert the exception type is 
> > InvalidOperationException so that we catch errors like if someone changes 
> > the thrown exception in the future.
> 
> Peter Vary wrote:
> Shall I separate the test case into two? With/Without cascade? There is a 
> little extra stuff there creating the table/function/index in two tests which 
> I wanted to avoid with one test case. That is why I decied to keep the 
> cascade tests in one test case and used try-catch here to check the exception 
> without the cascade option, and then proceed with the test and drop the 
> database with the cascade option.
> 
> What do you think?

I'd vote for separation. One test case for cascade and one for without cascade. 
The latter should have an @Test(expected = ..) annotation IMHO


- Adam


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


On Jan. 11, 2018, 12:54 p.m., Peter Vary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65018/
> ---
> 
> (Updated Jan. 11, 2018, 12:54 p.m.)
> 
> 
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-18372
> https://issues.apache.org/jira/browse/HIVE-18372
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Created:
> - AbstractMetastore class - to privide an interface for different metastore 
> implementation (start/stop/warehouse path methods)
> -- Implementation for Embedded/Remote/Cluster metastores
> - MiniHMS with builder - to create hms instances for test
> - MetaStoreFactory - to create the parameter list for parametrized test
> - TestDatabases - test for database related metastore functions to showcase 
> the infrastructure
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/MetaStoreFactoryForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/ClusterMetaStoreForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/EmbeddedMetaStoreForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/MiniHMS.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/RemoteMetaStoreForTests.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65018/diff/4/
> 
> 
> Testing
> ---
> 
> Run the new tests
> 
> 
> Thanks,
> 
> Peter Vary
> 
>

Re: Review Request 65018: HIVE-18372 Create testing infra to test different HMS instances

[Peter Vary via Review Board]

> On Jan. 10, 2018, 6:35 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
> > Lines 350 (patched)
> > 
> >
> > I think we should catch the exception and assert the exception type is 
> > InvalidOperationException so that we catch errors like if someone changes 
> > the thrown exception in the future.

Shall I separate the test case into two? With/Without cascade? There is a 
little extra stuff there creating the table/function/index in two tests which I 
wanted to avoid with one test case. That is why I decied to keep the cascade 
tests in one test case and used try-catch here to check the exception without 
the cascade option, and then proceed with the test and drop the database with 
the cascade option.

What do you think?


- Peter


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


On Jan. 11, 2018, 12:54 p.m., Peter Vary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65018/
> ---
> 
> (Updated Jan. 11, 2018, 12:54 p.m.)
> 
> 
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-18372
> https://issues.apache.org/jira/browse/HIVE-18372
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Created:
> - AbstractMetastore class - to privide an interface for different metastore 
> implementation (start/stop/warehouse path methods)
> -- Implementation for Embedded/Remote/Cluster metastores
> - MiniHMS with builder - to create hms instances for test
> - MetaStoreFactory - to create the parameter list for parametrized test
> - TestDatabases - test for database related metastore functions to showcase 
> the infrastructure
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/MetaStoreFactoryForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/ClusterMetaStoreForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/EmbeddedMetaStoreForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/MiniHMS.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/RemoteMetaStoreForTests.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65018/diff/4/
> 
> 
> Testing
> ---
> 
> Run the new tests
> 
> 
> Thanks,
> 
> Peter Vary
> 
>

Re: Review Request 65018: HIVE-18372 Create testing infra to test different HMS instances

[Peter Vary via Review Board]

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

(Updated Jan. 11, 2018, 12:54 p.m.)


Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
Karajgaonkar.


Changes
---

Updated the patch based on Vihang's comment


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


Repository: hive-git


Description
---

Created:
- AbstractMetastore class - to privide an interface for different metastore 
implementation (start/stop/warehouse path methods)
-- Implementation for Embedded/Remote/Cluster metastores
- MiniHMS with builder - to create hms instances for test
- MetaStoreFactory - to create the parameter list for parametrized test
- TestDatabases - test for database related metastore functions to showcase the 
infrastructure


Diffs (updated)
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/MetaStoreFactoryForTests.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/ClusterMetaStoreForTests.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/EmbeddedMetaStoreForTests.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/MiniHMS.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/RemoteMetaStoreForTests.java
 PRE-CREATION 


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

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


Testing
---

Run the new tests


Thanks,

Peter Vary

Re: Review Request 65018: HIVE-18372 Create testing infra to test different HMS instances

[Vihang Karajgaonkar via Review Board]

> On Jan. 10, 2018, 3:35 a.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
> > Lines 52 (patched)
> > 
> >
> > Parameterized tests don't work well in Ptest since it is cannot be 
> > parallelized. I would suggest creating a Abstract base class with all the 
> > tests and creating subclasses for remote and embedded like we have for 
> > TestHiveMetaStore test currently.
> 
> Peter Vary wrote:
> TestHiveMetaStore currently has 5 subclasses (TestEmbeddedHiveMetaStore, 
> TestRemoteHiveMetaStore, TestSetUGIOnBothClientServer, 
> TestSetUGIOnOnlyClient, TestSetUGIOnOnlyServer) running the same tests 
> against different configurations. This class is a huge one which should be 
> split, but spliting would mean to multiple the test classes 5 times. That is 
> why I think it is better to do this way.
> If we will have timing concerns then we still can split the test cases 
> more.
> 
> What do you think?

I think thats fine. If we find this test is taking a lot of time may be we 
should split it later. I was concerned because overtime folks would add more 
tests to these classes and they become huge. One of the optimizations which was 
done sometime back to reduce the Ptest execution time was to change some 
long-running parameterized classes to sub-classes so that they can be run in 
parallel. Having *more* number of test classes should not be a concern as long 
as we don't have duplicate code.


- Vihang


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


On Jan. 10, 2018, 3:44 p.m., Peter Vary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65018/
> ---
> 
> (Updated Jan. 10, 2018, 3:44 p.m.)
> 
> 
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-18372
> https://issues.apache.org/jira/browse/HIVE-18372
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Created:
> - AbstractMetastore class - to privide an interface for different metastore 
> implementation (start/stop/warehouse path methods)
> -- Implementation for Embedded/Remote/Cluster metastores
> - MiniHMS with builder - to create hms instances for test
> - MetaStoreFactory - to create the parameter list for parametrized test
> - TestDatabases - test for database related metastore functions to showcase 
> the infrastructure
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/MetaStoreFactoryForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/ClusterMetaStoreForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/EmbeddedMetaStoreForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/MiniHMS.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/RemoteMetaStoreForTests.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65018/diff/3/
> 
> 
> Testing
> ---
> 
> Run the new tests
> 
> 
> Thanks,
> 
> Peter Vary
> 
>

Re: Review Request 65018: HIVE-18372 Create testing infra to test different HMS instances

[Vihang Karajgaonkar via Review Board]

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


Fix it, then Ship it!




Overall looks good. Thanks for taking this up. I suggested some small 
improvements to the tests below.


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


may be we should catch the exception (and do nothing) here so that it 
atleast attempts to stop other metastores in the list.



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


should we check if the directory is removed after dropping the db here?



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


looks like this throws InvalidObjectException according to the annotation, 
so may be this comment is not needed? Also, curious to understand why the name 
is invalid :)



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


I think we should catch the exception and assert the exception type is 
InvalidOperationException so that we catch errors like if someone changes the 
thrown exception in the future.



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


may be we should confirm if the directory is removed after drop cascade is 
done.



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


same as above. Catch the exception and assert the exception type is 
InvalidOperationException



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


assert directory is cleaned up after this line.



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


shouldn't we check if default database is also being returned?


- Vihang Karajgaonkar


On Jan. 10, 2018, 3:44 p.m., Peter Vary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65018/
> ---
> 
> (Updated Jan. 10, 2018, 3:44 p.m.)
> 
> 
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-18372
> https://issues.apache.org/jira/browse/HIVE-18372
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Created:
> - AbstractMetastore class - to privide an interface for different metastore 
> implementation (start/stop/warehouse path methods)
> -- Implementation for Embedded/Remote/Cluster metastores
> - MiniHMS with builder - to create hms instances for test
> - MetaStoreFactory - to create the parameter list for parametrized test
> - TestDatabases - test for database related metastore functions to showcase 
> the infrastructure
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/MetaStoreFactoryForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/ClusterMetaStoreForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/EmbeddedMetaStoreForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/MiniHMS.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/RemoteMetaStoreForTests.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65018/diff/3/
> 
> 
> Testing
> ---
> 
> Run the new tests
> 
> 
> Thanks,
> 
> Peter Vary
> 
>

Re: Review Request 65018: HIVE-18372 Create testing infra to test different HMS instances

[Peter Vary via Review Board]

> On Jan. 10, 2018, 3:35 a.m., Vihang Karajgaonkar wrote:
> >

Thanks for the review Vihang!


> On Jan. 10, 2018, 3:35 a.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
> > Lines 52 (patched)
> > 
> >
> > Parameterized tests don't work well in Ptest since it is cannot be 
> > parallelized. I would suggest creating a Abstract base class with all the 
> > tests and creating subclasses for remote and embedded like we have for 
> > TestHiveMetaStore test currently.

TestHiveMetaStore currently has 5 subclasses (TestEmbeddedHiveMetaStore, 
TestRemoteHiveMetaStore, TestSetUGIOnBothClientServer, TestSetUGIOnOnlyClient, 
TestSetUGIOnOnlyServer) running the same tests against different 
configurations. This class is a huge one which should be split, but spliting 
would mean to multiple the test classes 5 times. That is why I think it is 
better to do this way.
If we will have timing concerns then we still can split the test cases more.

What do you think?


- Peter


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


On Jan. 10, 2018, 3:44 p.m., Peter Vary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65018/
> ---
> 
> (Updated Jan. 10, 2018, 3:44 p.m.)
> 
> 
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-18372
> https://issues.apache.org/jira/browse/HIVE-18372
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Created:
> - AbstractMetastore class - to privide an interface for different metastore 
> implementation (start/stop/warehouse path methods)
> -- Implementation for Embedded/Remote/Cluster metastores
> - MiniHMS with builder - to create hms instances for test
> - MetaStoreFactory - to create the parameter list for parametrized test
> - TestDatabases - test for database related metastore functions to showcase 
> the infrastructure
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/MetaStoreFactoryForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/ClusterMetaStoreForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/EmbeddedMetaStoreForTests.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/MiniHMS.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/RemoteMetaStoreForTests.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65018/diff/3/
> 
> 
> Testing
> ---
> 
> Run the new tests
> 
> 
> Thanks,
> 
> Peter Vary
> 
>

Re: Review Request 65018: HIVE-18372 Create testing infra to test different HMS instances

[Peter Vary via Review Board]

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

(Updated Jan. 10, 2018, 3:44 p.m.)


Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
Karajgaonkar.


Changes
---

Added more comments, and addessed Vihang's comments


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


Repository: hive-git


Description
---

Created:
- AbstractMetastore class - to privide an interface for different metastore 
implementation (start/stop/warehouse path methods)
-- Implementation for Embedded/Remote/Cluster metastores
- MiniHMS with builder - to create hms instances for test
- MetaStoreFactory - to create the parameter list for parametrized test
- TestDatabases - test for database related metastore functions to showcase the 
infrastructure


Diffs (updated)
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/MetaStoreFactoryForTests.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/ClusterMetaStoreForTests.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/EmbeddedMetaStoreForTests.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/MiniHMS.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/RemoteMetaStoreForTests.java
 PRE-CREATION 


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

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


Testing
---

Run the new tests


Thanks,

Peter Vary

Re: Review Request 65018: HIVE-18372 Create testing infra to test different HMS instances

[Vihang Karajgaonkar via Review Board]

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




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


If this is used for testing only I would suggest renaming it to something 
like MetaStoreFactoryForTests so that it is clear.

Same for the other new classes introduced namely, 
[Cluster|Embedded|Remote]Metastore.java



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


Parameterized tests don't work well in Ptest since it is cannot be 
parallelized. I would suggest creating a Abstract base class with all the tests 
and creating subclasses for remote and embedded like we have for 
TestHiveMetaStore test currently.


- Vihang Karajgaonkar


On Jan. 9, 2018, 2:50 p.m., Peter Vary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65018/
> ---
> 
> (Updated Jan. 9, 2018, 2:50 p.m.)
> 
> 
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-18372
> https://issues.apache.org/jira/browse/HIVE-18372
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Created:
> - AbstractMetastore class - to privide an interface for different metastore 
> implementation (start/stop/warehouse path methods)
> -- Implementation for Embedded/Remote/Cluster metastores
> - MiniHMS with builder - to create hms instances for test
> - MetaStoreFactory - to create the parameter list for parametrized test
> - TestDatabases - test for database related metastore functions to showcase 
> the infrastructure
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/MetaStoreFactory.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/ClusterMetaStore.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/EmbeddedMetaStore.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/MiniHMS.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/RemoteMetaStore.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65018/diff/2/
> 
> 
> Testing
> ---
> 
> Run the new tests
> 
> 
> Thanks,
> 
> Peter Vary
> 
>

Re: Review Request 65018: HIVE-18372 Create testing infra to test different HMS instances

[Peter Vary via Review Board]

> On Jan. 8, 2018, 3:15 p.m., Marta Kuczora wrote:
> > Thanks a lot for creating this testing infra for HMS testing!!! 
> > Overall it looks great to me, I would have only a few suggestions regarding 
> > the database tests. Please see my notes in the file.

Thanks for the review Marta!


> On Jan. 8, 2018, 3:15 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
> > Lines 168 (patched)
> > 
> >
> > I think it would be good to split this test into separate ones so each 
> > of them would test only one use-case, like
> > - creating db with null name
> > - creating db with invalid name
> > - creating db with empty name
> > This way it would be easier to see what use-case a test is supposed to 
> > test.
> > What do you think?

Done


> On Jan. 8, 2018, 3:15 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
> > Lines 282 (patched)
> > 
> >
> > I think this test could be also split (like the 
> > testCreateDatabaseInvalidData) into two separate tests:
> > - drop null db
> > - drop default db

Done


> On Jan. 8, 2018, 3:15 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
> > Lines 340 (patched)
> > 
> >
> > This could also be separated into two tests.
> > - drop with ignore unknown parameter true
> > - drop with ignore unknown parameter false

Done


> On Jan. 8, 2018, 3:15 p.m., Marta Kuczora wrote:
> > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
> > Lines 353 (patched)
> > 
> >
> > This could also be separated into multiple test cases.

Done


- Peter


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


On Jan. 9, 2018, 2:50 p.m., Peter Vary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65018/
> ---
> 
> (Updated Jan. 9, 2018, 2:50 p.m.)
> 
> 
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-18372
> https://issues.apache.org/jira/browse/HIVE-18372
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Created:
> - AbstractMetastore class - to privide an interface for different metastore 
> implementation (start/stop/warehouse path methods)
> -- Implementation for Embedded/Remote/Cluster metastores
> - MiniHMS with builder - to create hms instances for test
> - MetaStoreFactory - to create the parameter list for parametrized test
> - TestDatabases - test for database related metastore functions to showcase 
> the infrastructure
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/MetaStoreFactory.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/ClusterMetaStore.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/EmbeddedMetaStore.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/MiniHMS.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/RemoteMetaStore.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65018/diff/2/
> 
> 
> Testing
> ---
> 
> Run the new tests
> 
> 
> Thanks,
> 
> Peter Vary
> 
>

Re: Review Request 65018: HIVE-18372 Create testing infra to test different HMS instances

[Peter Vary via Review Board]

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

(Updated Jan. 9, 2018, 2:50 p.m.)


Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
Karajgaonkar.


Changes
---

Addressing Marta's comments


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


Repository: hive-git


Description
---

Created:
- AbstractMetastore class - to privide an interface for different metastore 
implementation (start/stop/warehouse path methods)
-- Implementation for Embedded/Remote/Cluster metastores
- MiniHMS with builder - to create hms instances for test
- MetaStoreFactory - to create the parameter list for parametrized test
- TestDatabases - test for database related metastore functions to showcase the 
infrastructure


Diffs (updated)
-

  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/MetaStoreFactory.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/ClusterMetaStore.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/EmbeddedMetaStore.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/MiniHMS.java
 PRE-CREATION 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/RemoteMetaStore.java
 PRE-CREATION 


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

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


Testing
---

Run the new tests


Thanks,

Peter Vary

Re: Review Request 65018: HIVE-18372 Create testing infra to test different HMS instances

[Marta Kuczora via Review Board]

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



Thanks a lot for creating this testing infra for HMS testing!!! 
Overall it looks great to me, I would have only a few suggestions regarding the 
database tests. Please see my notes in the file.


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


I think it would be good to split this test into separate ones so each of 
them would test only one use-case, like
- creating db with null name
- creating db with invalid name
- creating db with empty name
This way it would be easier to see what use-case a test is supposed to test.
What do you think?



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


I think this test could be also split (like the 
testCreateDatabaseInvalidData) into two separate tests:
- drop null db
- drop default db



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


It could also be checked with a list or getDB if the database is indeed 
dropped. What do you think?



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


This could also be separated into two tests.
- drop with ignore unknown parameter true
- drop with ignore unknown parameter false



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


This could also be separated into multiple test cases.


- Marta Kuczora


On Jan. 8, 2018, 12:19 p.m., Peter Vary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65018/
> ---
> 
> (Updated Jan. 8, 2018, 12:19 p.m.)
> 
> 
> Review request for hive, Alan Gates, Marta Kuczora, Adam Szita, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-18372
> https://issues.apache.org/jira/browse/HIVE-18372
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Created:
> - AbstractMetastore class - to privide an interface for different metastore 
> implementation (start/stop/warehouse path methods)
> -- Implementation for Embedded/Remote/Cluster metastores
> - MiniHMS with builder - to create hms instances for test
> - MetaStoreFactory - to create the parameter list for parametrized test
> - TestDatabases - test for database related metastore functions to showcase 
> the infrastructure
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/MetaStoreFactory.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/ClusterMetaStore.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/EmbeddedMetaStore.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/MiniHMS.java
>  PRE-CREATION 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/RemoteMetaStore.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65018/diff/1/
> 
> 
> Testing
> ---
> 
> Run the new tests
> 
> 
> Thanks,
> 
> Peter Vary
> 
>