Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13372 )
Change subject: IMPALA-8559 : Support config validation for event processor on HMS-3 ...................................................................... Patch Set 3: (16 comments) http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java@271 PS3, Line 271: public static List<MetastoreEventProcessorConfig> getEventProcessorConfigsToValidate() { Instead of using the shim,, did you consider just using an if statement around the shim's major version? I think that might be easier to follow, and the shim isn't really necessary here because this isn't accessing any classes from the Hive jars, right? http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@423 PS3, Line 423: * Hive-3 based environment missing "in a " http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@426 PS3, Line 426: return Arrays.asList(MetastoreEventProcessorConfig.FIRE_EVENTS_FOR_DML); worth a comment here explaining why we don't validate them all? http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/ConfigValidator.java File fe/src/main/java/org/apache/impala/catalog/events/ConfigValidator.java: http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/ConfigValidator.java@25 PS3, Line 25: class to caputuring the ValidationResult typos http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/ConfigValidator.java@37 PS3, Line 37: class ValidationResult { should be public if the interface is public http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/ConfigValidator.java@39 PS3, Line 39: // Boolean indicating whether the validation was success or failure. nit: use /** ... */ javadoc syntax here (I know Impala uses // a lot but that's wrong) http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/ConfigValidator.java@40 PS3, Line 40: private boolean valid_; can you make these variables final and mark the class @Immutable? http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/ConfigValidator.java@43 PS3, Line 43: private String reason_ = ""; no need for a initializer here since this is initted by the ctor http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/ConfigValidator.java@58 PS3, Line 58: return reason_; maybe Preconditions.checkState(!valid_) http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/DefaultConfigValidator.java File fe/src/main/java/org/apache/impala/catalog/events/DefaultConfigValidator.java: http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/DefaultConfigValidator.java@40 PS3, Line 40: Preconditions.checkNotNull(configKey); why use checkArgument for one and checkNotNull for the other? http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/DefaultConfigValidator.java@41 PS3, Line 41: this.expectedValue_ = expectedConfigValue; you could use = Preconditions.checkNotNull(expectedConfigValue) here and the next line, to cut out two lines of code http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/DefaultConfigValidator.java@50 PS3, Line 50: boolean result = expectedValue_.equalsIgnoreCase(configValue); : if (!result) { how about early return style here: if (expectedValue_.equalsIgnoreCase(configValue)) { return new ValidationResult(true, null); } http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/EventPropertyRegexValidator.java File fe/src/main/java/org/apache/impala/catalog/events/EventPropertyRegexValidator.java: http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/EventPropertyRegexValidator.java@40 PS3, Line 40: configKey this.configKey_ = Preconditions.checkNotNull(configKey) http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventProcessorConfig.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventProcessorConfig.java: http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventProcessorConfig.java@26 PS3, Line 26: * This is a super set of all the event processor configurations needed to be validated nit: add '.' at end of the sentence for better readability http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventProcessorConfig.java@40 PS3, Line 40: Preconditions.checkNotNull(configKey); I think this is redundant with the check you have on the ctor already http://gerrit.cloudera.org:8080/#/c/13372/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventProcessorConfig.java@46 PS3, Line 46: this.validator_ = validator; can combine prior two lines -- To view, visit http://gerrit.cloudera.org:8080/13372 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I35b3dd93f4a90c103a402349e8b9cd36db39a259 Gerrit-Change-Number: 13372 Gerrit-PatchSet: 3 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 23 May 2019 06:41:09 +0000 Gerrit-HasComments: Yes
