[
https://issues.apache.org/jira/browse/HIVE-17982?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16309601#comment-16309601
]
Peter Vary commented on HIVE-17982:
-----------------------------------
Thanks for all your work done in this patch.
I tried to review it, so we can continue on the standalone metastore work.
I surely missed some stuff, but found some questions:
- In several cases (for example {{TestRetryingHMSHandler}}) I see that
{{MetaStoreTestUtils.startMetaStoreWithRetry()}} is replaced by this code:
{code}
int port = MetaStoreTestUtils.findFreePort();
[..]
MetaStoreTestUtils.startMetaStoreWithRetry(port,
HadoopThriftAuthBridge.getBridge(), conf);
{code}
I am not sure that the original version was atomic (but at least we had the
possibility to create an atomic version of the method), but adding more code
between getting a free port and starting the metastore could cause racing
condition problems. Probably that is why the original version was "WithRetry".
- {{MetaStoreTestUtils.startMetaStoreWithRetry}} - I have concerns with this
change - see my previous comment
- {{TestHiveMetaStore}}
-- Line 153: I understand why you removed the {{assert(false)}} - I would
removed the try-catch too - the test should throw the exception if it is a
failure
-- Line 956, 1812, 3140: The original table definition contained bucketed cols
as well. The new table is not bucketed - maybe not important at all. Pointing
out just in case :)
-- Line 990 in the new file: nit: We might rely on the default value of the
{{ownerName}} too, or do not rely on default for the {{ownerType}} - just a
thought
-- Line 992 in the new file: nit: use only {{assertEquals}} instead
{{Assert.assertEquals}}, since it is already imported.
-- {{testGetSchemaWithNoClassDefFoundError}} - I do not understand the original
intent of this test - it seems like a very-very edge case (throwing a
NoClassDefFoundError when initializing the SerDe), but the new version does not
test the original case (it tests how the SerDe is not found).
-- Line 2131, 2143: I understand why you removed the {{assert(false)}} - I
would removed the try-catch too - the test should throw the exception if it is
a failure
-- Line 2156: nit: Why not drop database with cascade option?
-- Line 2916: nit: I am curious why this was run with retries
- {{TestMetaStoreEndFunctionListener}} in line 118, 132: Really every exception
means all good? I know it was there previously, but seem like a bad decision at
that time :)
- {{TestMetaStoreEventListener}} in line 245, 262, 282, 300, 313, 329, 368,
407, 429, 441,... in the new file: nit: use only {{assertEquals}} instead
{{Assert.assertEquals}}, since it is already imported.
Thanks,
Peter
> Move metastore specific itests
> ------------------------------
>
> Key: HIVE-17982
> URL: https://issues.apache.org/jira/browse/HIVE-17982
> Project: Hive
> Issue Type: Sub-task
> Components: Standalone Metastore
> Reporter: Alan Gates
> Assignee: Alan Gates
> Labels: pull-request-available
> Attachments: HIVE-17982.patch
>
>
> There are a number of tests in itests/hive-unit/.../metastore that are
> metastore specific. I suspect they were initially placed in itests only
> because the metastore pulling in a few plugins from ql.
> Given that we need to be able to release the metastore separately, we need to
> be able to test it completely as a standalone entity. So I propose to move a
> number of the itests over into standalone-metastore. I will only move tests
> that are isolated to the metastore. Anything that tests wider functionality
> I plan to leave in itests.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
