Bharath Krishna has posted comments on this change. (
http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Fetch metastore configuration values to
detect misconfigured setups
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File
fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@315
PS4, Line 315: try {
> Use try-with-resources here to automatically close MetastoreClient once don
Do you mean to use try-with-resource in the above method which calls using
MetastoreClient?
I added the client as an input param so that I can Mock the test and extract
out this method as independent.
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
File fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java:
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@88
PS4, Line 88: public static final String DEFAULT_METASTORE_CONFIG_VALUE =
> Why do we need this?
Will remove this if the below comment of mine doesn't sound like a good approach
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@123
PS4, Line 123: public static String getMetastoreConfigValue(
> May be a more useful way is to add a input argument to provide a default va
I feel this is useful too when users can just compare the return value with
MetastoreUtil.DEFAULT_METASTORE_CONFIG_VALUE
Do you feel it would be better to add two methods :
getMetastoreConfigValue(client, config) (which calls the below method with
DEFAULT_METASTORE_CONFIG_VALUE)
and
getMetastoreConfigValue(client, config, defaultVal)
Or should I just have the latter one?
http://gerrit.cloudera.org:8080/#/c/13019/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File
fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/13019/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@128
PS1, Line 128: import org.slf4j.LoggerFactory;
Use only the required imports
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File
fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/13019/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@253
PS4, Line 253: when(mockIMetaStoreClient.getConfigValue(config.toString(),
: MetaStoreUtil.DEFAULT_METASTORE_CONFIG_VALUE))
: .thenReturn(config.getExpectedValue());
> Why do we care to revert the value on a mock client?
Here, I am iterating over each Config, and just toggling its value. If I don't
revert the value back, in the next iteration it will throw an exception in the
previous value itself. Please let me know if it makes sense to do that way or
if should consider some other alternative.
--
To view, visit http://gerrit.cloudera.org:8080/13019
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94c2783e36287a65122003aa55d8075a806bc606
Gerrit-Change-Number: 13019
Gerrit-PatchSet: 4
Gerrit-Owner: Bharath Krishna <bhar...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <anu...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bhar...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Apr 2019 04:26:48 +0000
Gerrit-HasComments: Yes
