> 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 > > Sahil Takiar wrote: > 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?
About the abstract class: I try to avoid the abstract classes in tests when possible, because I think the typical usage of the tests is that a developer creates a patch, and a random test starts to break. Then the developer arrives a totally unfamiliar part of the code, and the less abstraction she meets, the more easily can she solve the issue. But To be honest I am not absolutely sure on this decision. I am ready the reopen this discussion after we committed the patches :) About stopping the metastore: I am not sure that stopping the metastore has any usages in production code. In these tests we start multiple metastore instances, so it would be good if we can stop them after the tests are finished, but the process is still running. > 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? > > Sahil Takiar wrote: > 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. Filed the jira: HIVE-18555 Inconsistent exceptions between different IMetaStoreClient implementations - Peter ----------------------------------------------------------- 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 > >
