Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations ...................................................................... Patch Set 7: (12 comments) Overall looks good. Few comments below and then good to go from my side. http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java File fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java: http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java@42 PS7, Line 42: EventProcessorValidation a better name could be EventProcessorConfigValidator http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java@42 PS7, Line 42: interface general convention in Impala is not to use package-private classes (although I think its a good idea to have them) For consistency, please change these to public. Same with the members of this class. Try to keep the members private unless its not possible. http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java@59 PS7, Line 59: getExpectedValue method doc http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java@74 PS7, Line 74: String private static final http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java@115 PS7, Line 115: MetastoreEventParameters nit, formatting is off http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java@120 PS7, Line 120: filtered s/filtered/filtered out/ http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidation.java@157 PS7, Line 157: Found would be good to provided the expected value here as well http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@68 PS7, Line 68: MetastoreEventParameters using parameters and properties interchangebly is confusing. may be rename this to MetastoreEventPropertyKey http://gerrit.cloudera.org:8080/#/c/13019/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@70 PS7, Line 70: CATALOG_VERSION_PROP_KEY _PROP_KEY is redundant if you name the enum as MetastoreEventPropertyKey. SAme for others. http://gerrit.cloudera.org:8080/#/c/13019/7/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/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@253 PS7, Line 253: Runtime error.. redundant to have this text here. can be removed. http://gerrit.cloudera.org:8080/#/c/13019/7/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/7/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@88 PS7, Line 88: DEFAULT_METASTORE_CONFIG_VALUE I am not convinced of the value of having this. Why can't we just use empty string which could be private to the config validator? http://gerrit.cloudera.org:8080/#/c/13019/7/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/7/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@336 PS7, Line 336: impala add a test case for ^impala as well since it matches the disableHmsSync paramter -- 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: 7 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: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Thu, 25 Apr 2019 20:23:32 +0000 Gerrit-HasComments: Yes
