> 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. > > Peter Vary wrote: > 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
I think its fine for now, its more of a nit. Regardless of the junit fix, can't all this logic still be pulled into an abstract class? Isn't stopping a remote metastore consist of just shutting down the process running HMS? > 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? > > Peter Vary wrote: > 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? Yeah, I was just curious. Its out of scope for this RB. I think making the making the two consistent is justifiable, even if it is backwards incompatible. We can make the change in Hive 3.0.0. Even if it comes after the HMS separation work, would still be good to have a JIRA to track it. - Sahil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65264/#review196144 ----------------------------------------------------------- On Jan. 25, 2018, 1:01 p.m., Peter Vary wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65264/ > ----------------------------------------------------------- > > (Updated Jan. 25, 2018, 1:01 p.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/TestTablesGetExists.java > PRE-CREATION > > standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestTablesList.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/65264/diff/4/ > > > Testing > ------- > > Run the created tests > > > Thanks, > > Peter Vary > >
