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