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