> On Jan. 24, 2018, 5:50 p.m., Sahil Takiar wrote: > >
Thanks for the review Sahil! > On Jan. 24, 2018, 5:50 p.m., Sahil Takiar wrote: > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesGetListExists.java > > Lines 63 (patched) > > <https://reviews.apache.org/r/65264/diff/3/?file=1944443#file1944443line63> > > > > A lot of the setup and teardown code for all these classes looks pretty > > similar. Is it worth creating an abstract class (something like > > `AbstractMetaStoreClientTest`) to encapsulate all the common setup / > > teardown logic? > > > > We don't have to do this for all the JIRAs in HIVE-18371, since most of > > them are already done. But would be good for future tests. When we have a junit release with this commit: https://github.com/junit-team/junit4/commit/1bf8438b65858565dbb64736bfe13aae9cfc1b5a Then we can change the code to something like this: ``` @Parameterized.Parameters(name = "{0}") public static List<Object[]> getMetaStoreToTest() throws Exception { return MetaStoreFactoryForTests.getMetaStores(); } public TestTablesList(String name, AbstractMetaStoreService metaStore) throws Exception { this.metaStore = metaStore; this.metaStore.start(); } @AfterParam public static void stopMetaStores() throws Exception { metaStore.stop(); } ``` Until then we need a way to stop the metastore services at the end of the runs. Sidenote: Currently metaStore.stop() does not do anything, since there is no clean way to stop a metastore. So really it is only there to remind us, that we should do something like this. Do you think we should remove the extra code, and file a jira to do this, when we have either the junit change in a release, or a way to stop a metastore? Thanks, Peter > On Jan. 24, 2018, 5:50 p.m., Sahil Takiar wrote: > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesGetListExists.java > > Lines 218-221 (patched) > > <https://reviews.apache.org/r/65264/diff/3/?file=1944443#file1944443line218> > > > > It's a bit weird that the embedded metastore and remote metastore throw > > different exceptions (speaking broadly here since this applies to a lot of > > the tests). > > > > Is there any way to fix that? Yeah, this "API" does many weird things :) Since the consensus on the dev list was, that we do not want to change the API, I do not see how can we solve this issue: - Embedded MetaStore should not throw TProtocolException - Remote MetaStore API is defined by the hive_metastore.thrift definition which defines this field as "required" We can change the IMetaStoreClient implementation, to check this before sending it to the MetaStore server, but that solution is even worse :( So I would skip this for now. What do you think? > On Jan. 24, 2018, 5:50 p.m., Sahil Takiar wrote: > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesGetListExists.java > > Lines 272 (patched) > > <https://reviews.apache.org/r/65264/diff/3/?file=1944443#file1944443line272> > > > > since only one exception is thrown, can use `expected` annotation. I have missed this. Thanks! > On Jan. 24, 2018, 5:50 p.m., Sahil Takiar wrote: > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesGetListExists.java > > Lines 338 (patched) > > <https://reviews.apache.org/r/65264/diff/3/?file=1944443#file1944443line338> > > > > should we just create a separate class for the list table APIs? they > > use a different set of tables anyway, and it might be good to split up the > > different filter tests Good idea! Done - Peter ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65264/#review196144 ----------------------------------------------------------- On Jan. 23, 2018, 11:18 a.m., Peter Vary wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65264/ > ----------------------------------------------------------- > > (Updated Jan. 23, 2018, 11:18 a.m.) > > > Review request for hive, Marta Kuczora, Adam Szita, and Vihang Karajgaonkar. > > > Bugs: HIVE-18481 > https://issues.apache.org/jira/browse/HIVE-18481 > > > Repository: hive-git > > > Description > ------- > > Create IMetaStoreClient tests to cover the table query methods > > > Diffs > ----- > > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesGetListExists.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/65264/diff/3/ > > > Testing > ------- > > Run the created tests > > > Thanks, > > Peter Vary > >
