[impala] 03/04: IMPALA-8761: Improve events processor configuration validation.

[st...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

stakiar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git

commit b2e5e942867ee1eeca754fd383075e7db3c8b590
Author: Anurag Mantripragada <an...@gmail.com>
AuthorDate: Mon Sep 16 13:55:37 2019 -0700

    IMPALA-8761: Improve events processor configuration validation.
    
    This patch aims to improve the validation of configuration keys
    from the HMS.
    
    IMPALA-8559 introduced configuration validation for events processor
    configurations. In the existing implementation, the events processor
    errors out as soon as it sees a validation failure. If there are
    more than one configuration errors, the users may have to restart
    HMS each time they fix a configuration error. This is bad user
    experience. This change collects all the configuration issues and
    logs expected values before erroring out. Users can now fix all
    issues in one go.
    
    Testing:
    Added testValidateConfigs() to assert if multiple incorrect values
    are detected at once.
    
    Change-Id: I73480872ef93215d05c1fd922e64eb68a8a63a42
    Reviewed-on: http://gerrit.cloudera.org:8080/14240
    Reviewed-by: Bharath Vissapragada <bh...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 .../catalog/events/MetastoreEventsProcessor.java   | 21 +++++++++++++----
 .../events/MetastoreEventsProcessorTest.java       | 27 ++++++++++++++++++++++
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java b/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
index 72e77e3..22c1237 100644
--- a/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
+++ b/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
@@ -23,6 +23,7 @@ import com.codahale.metrics.Timer;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
@@ -258,24 +259,34 @@ public class MetastoreEventsProcessor implements ExternalEventsProcessor {
    * Fetches the required metastore config values and validates them against the
    * expected values. The configurations to validate are different for HMS-2 v/s HMS-3
    * and hence it uses MetastoreShim to get the configurations which need to be validated.
-   * @throws CatalogException if the validation fails or if metastore is not accessible
+   * @throws CatalogException if one or more validations fail or if metastore is not
+   * accessible
    */
   @VisibleForTesting
   void validateConfigs() throws CatalogException {
+    List<ValidationResult> validationErrors = new ArrayList<>();
     for (MetastoreEventProcessorConfig config : getEventProcessorConfigsToValidate()) {
       String configKey = config.getValidator().getConfigKey();
       try {
         String value = getConfigValueFromMetastore(configKey, "");
         ValidationResult result = config.validate(value);
-        if (!result.isValid()) {
-          throw new CatalogException(result.getReason());
-        }
+        if (!result.isValid()) validationErrors.add(result);
       } catch (TException e) {
         String msg = String.format("Unable to get configuration %s from metastore. Check "
             + "if metastore is accessible", configKey);
         LOG.error(msg, e);
         throw new CatalogException(msg);
       }
+      if (!validationErrors.isEmpty()) {
+        LOG.error("Found {} incorrect metastore configuration(s).",
+            validationErrors.size());
+        for (ValidationResult invalidConfig: validationErrors) {
+          LOG.error(invalidConfig.getReason());
+        }
+        throw new CatalogException(String.format("Found %d incorrect metastore "
+            + "configuration(s). Events processor cannot start. See ERROR log for more "
+            + "details.", validationErrors.size()));
+      }
     }
   }
 
@@ -283,7 +294,7 @@ public class MetastoreEventsProcessor implements ExternalEventsProcessor {
    * Returns the list of Metastore configurations to validate depending on the hive
    * version
    */
-  private List<MetastoreEventProcessorConfig> getEventProcessorConfigsToValidate() {
+  public static List<MetastoreEventProcessorConfig> getEventProcessorConfigsToValidate() {
     if (MetastoreShim.getMajorVersion() >= 2) {
       return Arrays.asList(MetastoreEventProcessorConfig.FIRE_EVENTS_FOR_DML);
     }
diff --git a/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java b/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
index b496d5c..d79d550 100644
--- a/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
+++ b/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
@@ -136,6 +136,7 @@ import org.junit.Test;
 
 import com.google.common.collect.Lists;
 
+import org.mockito.Mockito;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -238,6 +239,32 @@ public class MetastoreEventsProcessorTest {
   }
 
   /**
+   * Test if validateConfigs() works as expected. If more than one configuration keys
+   * in metastore are incorrect, we present all of those incorrect results together so
+   * users can change them in one go. This test will assert the number of failed
+   * configurations for Hive major version >=2 or otherwise.
+   * @throws TException
+   */
+  @Test
+  public void testValidateConfig() throws TException {
+    MetastoreEventsProcessor mockMetastoreEventsProcessor =
+        Mockito.spy(eventsProcessor_);
+    for (MetastoreEventProcessorConfig config: MetastoreEventProcessorConfig.values()) {
+      String configKey = config.getValidator().getConfigKey();
+      Mockito.when(mockMetastoreEventsProcessor.getConfigValueFromMetastore(configKey,
+          "")).thenReturn("false");
+    }
+    try {
+      mockMetastoreEventsProcessor.validateConfigs();
+    } catch (CatalogException e) {
+      String errorMessage = "Found %d incorrect metastore configuration(s).";
+      // Use MetastoreShim to determine the number of failed configs.
+      assertTrue(e.getMessage().contains(String.format(errorMessage,
+          MetastoreEventsProcessor.getEventProcessorConfigsToValidate().size())));
+    }
+  }
+
+  /**
    * Test provides a mock value and confirms if the MetastoreEventConfig validate
    * suceeds or fails as expected
    */