Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13019 )
Change subject: IMPALA-8419 : Validate event processing related configurations ...................................................................... Patch Set 9: (25 comments) In general CR looks good, feedback is mostly about nits and Impala code style conformance. http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java File fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java: http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@51 PS9, Line 51: private String conf, expectedValue; make these final and use _ suffix since that's Impala convention for member variables. http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@62 PS9, Line 62: public String getExpectedValue() { Put this in one line, see the comment below. http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@69 PS9, Line 69: public String getConf() { : return conf; : } Impala code style to put this into one line when possible: public String getConf() { return conf_; } http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@79 PS9, Line 79: // Hive config name that can have regular expressions to filter out parameters. : String METASTORE_PARAMETER_EXCLUDE_PATTERNS = : "hive.metastore.notification.parameters.exclude.patterns"; : : // Default config value (currently just an empty String) to be returned from Metastore : // when the given config is not set. : String DEFAULT_METASTORE_CONFIG_VALUE = ""; should these be static final? http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@112 PS9, Line 112: static ValidationResult validateMetastoreEventParameters( : MetastoreEventsProcessor eventsProcessor) { add @VisibleForTesting since it looks like it's not private because you want to test it? http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@182 PS9, Line 182: class ValidationResult the general recommendation is to put this as an inner class to EventProcessorValidator http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@184 PS9, Line 184: // Boolean indicating whether the validation was success or failure. : private boolean valid; : // Reason indicating the failure of validation. Only relevant when the validation fails. : private String reason; use _ suffix naming convention http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@190 PS9, Line 190: this.valid = valid; add Preconditions.checkNotNull(valid) since "valid" can't be null http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/EventProcessorValidator.java@195 PS9, Line 195: this.valid = true; nit: ValidationResult(true, null) is probably better http://gerrit.cloudera.org:8080/#/c/13019/9/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/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@77 PS9, Line 77: : private String key; same comment about suffix naming convention http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@80 PS9, Line 80: public String getKey() { : return key; : } nit: usually method comes before the constructor http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@431 PS9, Line 431: * returns : * the database property nit: we can join these two lines http://gerrit.cloudera.org:8080/#/c/13019/9/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/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@251 PS9, Line 251: result.getReason().isPresent() ? : result.getReason().get() : : "Event Processor initialization failed during validation check." nit: result.getReason.orElse() is shorter http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@304 PS9, Line 304: @VisibleForTesting nit: add one extra space http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@617 PS9, Line 617: nit: remove 4 spaces http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3094 PS9, Line 3094: .getKey() nit: this can be moved to L3093 http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3109 PS9, Line 3109: .getKey())); nit: this can be moved to L3108 http://gerrit.cloudera.org:8080/#/c/13019/9/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/9/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@117 PS9, Line 117: public static String getMetastoreConfigValue( : IMetaStoreClient client, String config, String defaultVal) : throws TException { : return client.getConfigValue(config, defaultVal); : } fix indentation http://gerrit.cloudera.org:8080/#/c/13019/9/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/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@159 PS9, Line 159: private String conf, correctValue, incorrectValue; same comment about suffix naming convention http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@274 PS9, Line 274: assertTrue(testResult.getReason().isPresent()); maybe also assert the error message? http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@276 PS9, Line 276: when(mockEventsProcessor.getConfigValueFromMetastore(config.conf, : DEFAULT_METASTORE_CONFIG_VALUE)) : .thenReturn(config.correctValue); not quite sure the point of this http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@290 PS9, Line 290: MetastoreEventsProcessor mockEventsProcessor = : Mockito.mock(MetastoreEventsProcessor.class); : : //Regex to filter all parameters starting with impala : when(mockEventsProcessor.getConfigValueFromMetastore( : EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS, : DEFAULT_METASTORE_CONFIG_VALUE)).thenReturn("^impala"); : ValidationResult testResult = : validateMetastoreEventParameters(mockEventsProcessor); : assertFalse(testResult.isValid()); : assertTrue(testResult.getReason().isPresent()); : : when(mockEventsProcessor.getConfigValueFromMetastore( : EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS, : DEFAULT_METASTORE_CONFIG_VALUE)).thenReturn("impala*"); : testResult = : validateMetastoreEventParameters(mockEventsProcessor); : assertFalse(testResult.isValid()); : assertTrue(testResult.getReason().isPresent()); : : when(mockEventsProcessor.getConfigValueFromMetastore( : EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS, : DEFAULT_METASTORE_CONFIG_VALUE)).thenReturn(""); : testResult = : validateMetastoreEventParameters(mockEventsProcessor); : assertTrue(testResult.isValid()); : assertFalse(testResult.getReason().isPresent()); : : // Test with default return value : when(mockEventsProcessor.getConfigValueFromMetastore( : EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS, : DEFAULT_METASTORE_CONFIG_VALUE)) : .thenReturn(DEFAULT_METASTORE_CONFIG_VALUE); : testResult = : validateMetastoreEventParameters(mockEventsProcessor); : assertTrue(testResult.isValid()); : assertFalse(testResult.getReason().isPresent()); : : when(mockEventsProcessor.getConfigValueFromMetastore( : EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS, : DEFAULT_METASTORE_CONFIG_VALUE)).thenReturn("randomString1, impala" : + ".disableHmsSync, randomString2"); : testResult = : validateMetastoreEventParameters(mockEventsProcessor); : assertFalse(testResult.isValid()); : assertTrue(testResult.getReason().isPresent()); : : when(mockEventsProcessor.getConfigValueFromMetastore( : EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS, : DEFAULT_METASTORE_CONFIG_VALUE)) : .thenReturn("impala.events.catalogServiceId"); : testResult = : validateMetastoreEventParameters(mockEventsProcessor); : assertFalse(testResult.isValid()); : assertTrue(testResult.getReason().isPresent()); : : when(mockEventsProcessor.getConfigValueFromMetastore( : EventProcessorValidator.METASTORE_PARAMETER_EXCLUDE_PATTERNS, : DEFAULT_METASTORE_CONFIG_VALUE)).thenReturn("^impala.events" : + ".catalogServiceId"); : testResult = : validateMetastoreEventParameters(mockEventsProcessor); : assertFalse(testResult.isValid()); : assertTrue(testResult.getReason().isPresent()); assert the error messages as well? http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@354 PS9, Line 354: nit: remove extra empty new line http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@777 PS9, Line 777: nit: remove 4 spaces http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/SynchronousHMSEventProcessorForTests.java File fe/src/test/java/org/apache/impala/catalog/events/SynchronousHMSEventProcessorForTests.java: http://gerrit.cloudera.org:8080/#/c/13019/9/fe/src/test/java/org/apache/impala/catalog/events/SynchronousHMSEventProcessorForTests.java@30 PS9, Line 30: nit: too overly indented, remove 4 spaces -- 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: 9 Gerrit-Owner: Bharath Krishna <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Bharath Krishna <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Mon, 29 Apr 2019 16:03:52 +0000 Gerrit-HasComments: Yes
