You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Peter Vary via Review Board <no...@reviews.apache.org> on 2018/01/08 12:19:20 UTC
Review Request 65018: HIVE-18372 Create testing infra to test
different HMS instances
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65018/
-----------------------------------------------------------
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
Re: Review Request 65018: HIVE-18372 Create testing infra to test
different HMS instances
Posted by Vihang Karajgaonkar via Review Board <no...@reviews.apache.org>.
> 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)
> > <https://reviews.apache.org/r/65018/diff/2/?file=1937641#file1937641line52>
> >
> > 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
Posted by Peter Vary via Review Board <no...@reviews.apache.org>.
> 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)
> > <https://reviews.apache.org/r/65018/diff/2/?file=1937641#file1937641line52>
> >
> > 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
Posted by Vihang Karajgaonkar via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/65018/#comment274255>
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)
<https://reviews.apache.org/r/65018/#comment274256>
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
Posted by Adam Szita via Review Board <no...@reviews.apache.org>.
> 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)
> > <https://reviews.apache.org/r/65018/diff/3/?file=1938268#file1938268line350>
> >
> > 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
Posted by Peter Vary via Review Board <no...@reviews.apache.org>.
> 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)
> > <https://reviews.apache.org/r/65018/diff/3/?file=1938268#file1938268line350>
> >
> > 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
Posted by Peter Vary via Review Board <no...@reviews.apache.org>.
> 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)
> > <https://reviews.apache.org/r/65018/diff/3/?file=1938268#file1938268line350>
> >
> > 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
Posted by Vihang Karajgaonkar via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/65018/#comment274297>
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)
<https://reviews.apache.org/r/65018/#comment274298>
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)
<https://reviews.apache.org/r/65018/#comment274299>
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)
<https://reviews.apache.org/r/65018/#comment274300>
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)
<https://reviews.apache.org/r/65018/#comment274301>
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)
<https://reviews.apache.org/r/65018/#comment274302>
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)
<https://reviews.apache.org/r/65018/#comment274303>
assert directory is cleaned up after this line.
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestDatabases.java
Lines 433 (patched)
<https://reviews.apache.org/r/65018/#comment274304>
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
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/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
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/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
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/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
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/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
Posted by Peter Vary via Review Board <no...@reviews.apache.org>.
> 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)
> > <https://reviews.apache.org/r/65018/diff/1/?file=1935455#file1935455line168>
> >
> > 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)
> > <https://reviews.apache.org/r/65018/diff/1/?file=1935455#file1935455line282>
> >
> > 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)
> > <https://reviews.apache.org/r/65018/diff/1/?file=1935455#file1935455line340>
> >
> > 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)
> > <https://reviews.apache.org/r/65018/diff/1/?file=1935455#file1935455line353>
> >
> > 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
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/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)
<https://reviews.apache.org/r/65018/#comment274044>
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)
<https://reviews.apache.org/r/65018/#comment274045>
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)
<https://reviews.apache.org/r/65018/#comment274046>
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)
<https://reviews.apache.org/r/65018/#comment274047>
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)
<https://reviews.apache.org/r/65018/#comment274048>
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
>
>