drill-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From j...@apache.org
Subject [5/7] drill git commit: DRILL-3496: Updates from review comments
Date Tue, 04 Aug 2015 22:11:25 GMT
DRILL-3496: Updates from review comments

DRILL-3496: Update: Multiple log msgs. -> single, aligned multi-line message.

Main: Changed avoidance of original single hard-to-read list from
first draft's multiple single-item log calls (resulting in multiple log
calls/entries) to single log call with aligned multi-line message.

Also revised message wording, etc.

DRILL-3496: Update 2: Some DEBUG -> INFO; another single multi-line msg.

- logger.debug(...) -> logger.info(...) for config. file messages
- changed drill-module.conf to single multi-line message like
  scanForImplementations cases

DRILL-3496: Update 3:  Various review comment responses.

- Removed isTraceEnabled/etc. guards.
- Simplified logging line break/indentation code.
- Added some more "final".
- Undid logger cleanup. :-(


Project: http://git-wip-us.apache.org/repos/asf/drill/repo
Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/5cd99162
Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/5cd99162
Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/5cd99162

Branch: refs/heads/master
Commit: 5cd991628b79f64b681f1eb18deb5858b4264774
Parents: d144bdc
Author: dbarclay <dbarclay@maprtech.com>
Authored: Tue Jul 14 23:37:14 2015 -0700
Committer: Jason Altekruse <altekrusejason@gmail.com>
Committed: Tue Aug 4 08:49:02 2015 -0700

----------------------------------------------------------------------
 .../apache/drill/common/config/DrillConfig.java | 39 +++++++++-----------
 .../common/logical/FormatPluginConfigBase.java  | 19 ++++++----
 .../common/logical/StoragePluginConfigBase.java | 21 ++++++-----
 .../logical/data/LogicalOperatorBase.java       | 11 ++++--
 .../apache/drill/common/util/PathScanner.java   | 10 +++--
 .../physical/base/PhysicalOperatorUtil.java     | 17 +++++----
 .../physical/impl/OperatorCreatorRegistry.java  |  6 +--
 .../drill/exec/store/StoragePluginRegistry.java | 17 ++++++---
 8 files changed, 81 insertions(+), 59 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/drill/blob/5cd99162/common/src/main/java/org/apache/drill/common/config/DrillConfig.java
----------------------------------------------------------------------
diff --git a/common/src/main/java/org/apache/drill/common/config/DrillConfig.java b/common/src/main/java/org/apache/drill/common/config/DrillConfig.java
index e5f0278..46fc7f9 100644
--- a/common/src/main/java/org/apache/drill/common/config/DrillConfig.java
+++ b/common/src/main/java/org/apache/drill/common/config/DrillConfig.java
@@ -34,8 +34,6 @@ import org.apache.drill.common.logical.StoragePluginConfigBase;
 import org.apache.drill.common.logical.data.LogicalOperatorBase;
 import org.apache.drill.common.util.PathScanner;
 import org.reflections.util.ClasspathHelper;
-import org.slf4j.Logger;
-import static org.slf4j.LoggerFactory.getLogger;
 
 import com.fasterxml.jackson.core.JsonGenerator;
 import com.fasterxml.jackson.core.JsonParser.Feature;
@@ -43,13 +41,14 @@ import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.SerializationFeature;
 import com.fasterxml.jackson.databind.module.SimpleModule;
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.typesafe.config.Config;
 import com.typesafe.config.ConfigFactory;
 import com.typesafe.config.ConfigRenderOptions;
 
 public final class DrillConfig extends NestedConfig{
-  private static final Logger logger = getLogger(DrillConfig.class);
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(DrillConfig.class);
 
   private final ObjectMapper mapper;
   private final ImmutableList<String> startupArguments;
@@ -66,10 +65,8 @@ public final class DrillConfig extends NestedConfig{
   public DrillConfig(Config config, boolean enableServerConfigs) {
     super(config);
     logger.debug("Setting up DrillConfig object.");
-    if (logger.isTraceEnabled()) {
-      logger.trace("Given Config object is:\n{}",
-                   config.root().render(ConfigRenderOptions.defaults()));
-    }
+    logger.trace("Given Config object is:\n{}",
+                 config.root().render(ConfigRenderOptions.defaults()));
     mapper = new ObjectMapper();
 
     if (enableServerConfigs) {
@@ -77,7 +74,6 @@ public final class DrillConfig extends NestedConfig{
         .addDeserializer(LogicalExpression.class, new LogicalExpression.De(this))
         .addDeserializer(SchemaPath.class, new SchemaPath.De(this));
 
-
       mapper.registerModule(deserModule);
       mapper.enable(SerializationFeature.INDENT_OUTPUT);
       mapper.configure(Feature.ALLOW_UNQUOTED_FIELD_NAMES, true);
@@ -99,7 +95,7 @@ public final class DrillConfig extends NestedConfig{
 
   /**
    * Creates a DrillConfig object using the default config file name
-   * and with server-specific configuration options disabled.
+   * and with server-specific configuration options enabled.
    * @return The new DrillConfig object.
    */
   public static DrillConfig create() {
@@ -139,8 +135,9 @@ public final class DrillConfig extends NestedConfig{
    * @param overrideFileResourcePathname
    *          the classpath resource pathname of the file to use for
    *          configuration override purposes; {@code null} specifies to use the
-   *          default pathname ({@link CommonConstants.CONFIG_OVERRIDE})
-   *          (<strong>not</strong> to not look for an override file at all)
+   *          default pathname ({@link CommonConstants.CONFIG_OVERRIDE}) (does
+   *          <strong>not</strong> specify to suppress trying to load an
+   *          overrides file)
    *  @return A merged Config object.
    */
   public static DrillConfig create(String overrideFileResourcePathname) {
@@ -156,7 +153,6 @@ public final class DrillConfig extends NestedConfig{
   }
 
   /**
-   * ...
    * @param overrideFileResourcePathname
    *          see {@link #create(String)}'s {@code overrideFileResourcePathname}
    */
@@ -165,7 +161,6 @@ public final class DrillConfig extends NestedConfig{
   }
 
   /**
-   * ...
    * @param overrideFileResourcePathname
    *          see {@link #create(String)}'s {@code overrideFileResourcePathname}
    * @param overriderProps
@@ -190,7 +185,7 @@ public final class DrillConfig extends NestedConfig{
       final URL url =
           classLoader.getResource(CommonConstants.CONFIG_DEFAULT_RESOURCE_PATHNAME);
       if (null != url) {
-        logger.debug("Loading base config. file at {}.", url);
+        logger.info("Loading base configuration file at {}.", url);
         fallback =
             ConfigFactory.load(classLoader,
                                CommonConstants.CONFIG_DEFAULT_RESOURCE_PATHNAME);
@@ -199,28 +194,30 @@ public final class DrillConfig extends NestedConfig{
     }
 
     // 2. Load per-module configuration files.
-    Collection<URL> urls = PathScanner.getConfigURLs();
-    logger.debug("Loading configs at the following URLs {}", urls);
+    final Collection<URL> urls = PathScanner.getConfigURLs();
+    final String lineBrokenList =
+        urls.size() == 0 ? "" : "\n\t- " + Joiner.on("\n\t- ").join(urls);
+    logger.info("Loading {} module configuration files at: {}.",
+                urls.size(), lineBrokenList);
     for (URL url : urls) {
-      logger.debug("Loading module config. file at {}.", url);
       fallback = ConfigFactory.parseURL(url).withFallback(fallback);
     }
 
-    // 3. Load any overrides configuration file and overrides from JVM
-    // system properties (e.g., {-Dname=value").
+    // 3. Load any specified overrides configuration file along with any
+    //    overrides from JVM system properties (e.g., {-Dname=value").
 
     // (Per ConfigFactory.load(...)'s mention of using Thread.getContextClassLoader():)
     final URL overrideFileUrl =
         Thread.currentThread().getContextClassLoader().getResource(overrideFileResourcePathname);
     if (null != overrideFileUrl ) {
-      logger.debug("Loading override config. file at {}.", overrideFileUrl);
+      logger.info("Loading override config. file at {}.", overrideFileUrl);
     }
     Config effectiveConfig =
         ConfigFactory.load(overrideFileResourcePathname).withFallback(fallback);
 
     // 4. Apply any overriding properties.
     if (overriderProps != null) {
-      logger.debug("Loading override Properties parameter {}.", overriderProps);
+      logger.info("Loading override Properties parameter {}.", overriderProps);
       effectiveConfig =
           ConfigFactory.parseProperties(overriderProps).withFallback(effectiveConfig);
     }

http://git-wip-us.apache.org/repos/asf/drill/blob/5cd99162/common/src/main/java/org/apache/drill/common/logical/FormatPluginConfigBase.java
----------------------------------------------------------------------
diff --git a/common/src/main/java/org/apache/drill/common/logical/FormatPluginConfigBase.java
b/common/src/main/java/org/apache/drill/common/logical/FormatPluginConfigBase.java
index 1cd5dce..02f3206 100644
--- a/common/src/main/java/org/apache/drill/common/logical/FormatPluginConfigBase.java
+++ b/common/src/main/java/org/apache/drill/common/logical/FormatPluginConfigBase.java
@@ -23,8 +23,11 @@ import org.apache.drill.common.config.CommonConstants;
 import org.apache.drill.common.config.DrillConfig;
 import org.apache.drill.common.util.PathScanner;
 
+import com.google.common.base.Joiner;
+
+
 public abstract class FormatPluginConfigBase implements FormatPluginConfig{
-  static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FormatPluginConfigBase.class);
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FormatPluginConfigBase.class);
 
 
   /**
@@ -33,14 +36,16 @@ public abstract class FormatPluginConfigBase implements FormatPluginConfig{
    * @param config - Drill configuration object, used to find the packages to scan
    * @return - list of classes that implement the interface.
    */
-  public synchronized static Class<?>[] getSubTypes(DrillConfig config) {
-    List<String> packages =
+  public synchronized static Class<?>[] getSubTypes(final DrillConfig config) {
+    final List<String> packages =
         config.getStringList(CommonConstants.STORAGE_PLUGIN_CONFIG_SCAN_PACKAGES);
-    Class<?>[] pluginClasses =
+    final Class<?>[] pluginClasses =
         PathScanner.scanForImplementationsArr(FormatPluginConfig.class, packages);
-    for ( Class<?> pluginClass : pluginClasses ) {
-      logger.debug("Adding format plugin configuration {}", (Object) pluginClass );
-    }
+    final String lineBrokenList =
+        pluginClasses.length == 0
+        ? "" : "\n\t- " + Joiner.on("\n\t- ").join(pluginClasses);
+    logger.debug("Found {} format plugin configuration classes: {}.",
+                 pluginClasses.length, lineBrokenList);
     return pluginClasses;
   }
 

http://git-wip-us.apache.org/repos/asf/drill/blob/5cd99162/common/src/main/java/org/apache/drill/common/logical/StoragePluginConfigBase.java
----------------------------------------------------------------------
diff --git a/common/src/main/java/org/apache/drill/common/logical/StoragePluginConfigBase.java
b/common/src/main/java/org/apache/drill/common/logical/StoragePluginConfigBase.java
index dd9b18a..3ac858b 100644
--- a/common/src/main/java/org/apache/drill/common/logical/StoragePluginConfigBase.java
+++ b/common/src/main/java/org/apache/drill/common/logical/StoragePluginConfigBase.java
@@ -23,20 +23,23 @@ import org.apache.drill.common.config.CommonConstants;
 import org.apache.drill.common.config.DrillConfig;
 import org.apache.drill.common.util.PathScanner;
 
-import org.slf4j.Logger;
-import static org.slf4j.LoggerFactory.getLogger;
+import com.google.common.base.Joiner;
+
 
 public abstract class StoragePluginConfigBase extends StoragePluginConfig {
-  private static final Logger logger = getLogger(StoragePluginConfigBase.class);
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(StoragePluginConfigBase.class);
+
 
-  public synchronized static Class<?>[] getSubTypes(DrillConfig config){
-    List<String> packages =
+  public synchronized static Class<?>[] getSubTypes(final DrillConfig config) {
+    final List<String> packages =
         config.getStringList(CommonConstants.STORAGE_PLUGIN_CONFIG_SCAN_PACKAGES);
-    Class<?>[] pluginClasses =
+    final Class<?>[] pluginClasses =
         PathScanner.scanForImplementationsArr(StoragePluginConfig.class, packages);
-    for (Class<?> pluginClass : pluginClasses) {
-      logger.debug("Adding storage plugin configuration {}", (Object) pluginClass );
-    }
+    final String lineBrokenList =
+        pluginClasses.length == 0
+        ? "" : "\n\t- " + Joiner.on("\n\t- ").join(pluginClasses);
+    logger.debug("Found {} storage plugin configuration classes: {}.",
+                 pluginClasses.length, lineBrokenList);
     return pluginClasses;
   }
 

http://git-wip-us.apache.org/repos/asf/drill/blob/5cd99162/common/src/main/java/org/apache/drill/common/logical/data/LogicalOperatorBase.java
----------------------------------------------------------------------
diff --git a/common/src/main/java/org/apache/drill/common/logical/data/LogicalOperatorBase.java
b/common/src/main/java/org/apache/drill/common/logical/data/LogicalOperatorBase.java
index 647fcf6..e97ef19 100644
--- a/common/src/main/java/org/apache/drill/common/logical/data/LogicalOperatorBase.java
+++ b/common/src/main/java/org/apache/drill/common/logical/data/LogicalOperatorBase.java
@@ -30,6 +30,8 @@ import org.apache.drill.common.util.PathScanner;
 import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.annotation.JsonInclude.Include;
 import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Joiner;
+
 
 public abstract class LogicalOperatorBase implements LogicalOperator{
   static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(LogicalOperatorBase.class);
@@ -84,14 +86,15 @@ public abstract class LogicalOperatorBase implements LogicalOperator{
     this.memo = memo;
   }
 
-  public synchronized static Class<?>[] getSubTypes(DrillConfig config) {
+  public synchronized static Class<?>[] getSubTypes(final DrillConfig config) {
     final List<String> packages =
         config.getStringList(CommonConstants.LOGICAL_OPERATOR_SCAN_PACKAGES);
     final Class<?>[] ops =
         PathScanner.scanForImplementationsArr(LogicalOperator.class, packages);
-    for ( Class<?> op : ops ) {
-      logger.debug("Adding logical operator {}", (Object) op);
-    }
+    final String lineBrokenList =
+        ops.length == 0 ? "" : "\n\t- " + Joiner.on("\n\t- ").join(ops);
+    logger.debug("Found {} logical operator classes: {}.", ops.length,
+                 lineBrokenList);
     return ops;
   }
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/5cd99162/common/src/main/java/org/apache/drill/common/util/PathScanner.java
----------------------------------------------------------------------
diff --git a/common/src/main/java/org/apache/drill/common/util/PathScanner.java b/common/src/main/java/org/apache/drill/common/util/PathScanner.java
index 9832ae9..16537b6 100644
--- a/common/src/main/java/org/apache/drill/common/util/PathScanner.java
+++ b/common/src/main/java/org/apache/drill/common/util/PathScanner.java
@@ -37,14 +37,12 @@ import org.reflections.scanners.SubTypesScanner;
 import org.reflections.scanners.TypeAnnotationsScanner;
 import org.reflections.util.ClasspathHelper;
 import org.reflections.util.ConfigurationBuilder;
-import org.slf4j.Logger;
-import static org.slf4j.LoggerFactory.getLogger;
 
 import com.google.common.base.Stopwatch;
 import com.google.common.collect.Sets;
 
 public class PathScanner {
-  private static final Logger logger = getLogger(PathScanner.class);
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(PathScanner.class);
 
   private static final SubTypesScanner subTypeScanner = new SubTypesScanner();
   private static final TypeAnnotationsScanner annotationsScanner = new TypeAnnotationsScanner();
@@ -81,6 +79,9 @@ public class PathScanner {
     return REFLECTIONS;
   }
 
+  /**
+   * @param  scanPackages  note:  not currently used
+   */
   public static <T> Class<?>[] scanForImplementationsArr(final Class<T>
baseClass,
                                                          final List<String> scanPackages)
{
     Collection<Class<? extends T>> imps = scanForImplementations(baseClass, scanPackages);
@@ -88,6 +89,9 @@ public class PathScanner {
     return arr;
   }
 
+  /**
+   * @param  scanPackages  note:  not currently used
+   */
   public static <T> Set<Class<? extends T>> scanForImplementations(final
Class<T> baseClass,
                                                                    final List<String>
scanPackages) {
     final Stopwatch w = new Stopwatch().start();

http://git-wip-us.apache.org/repos/asf/drill/blob/5cd99162/exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalOperatorUtil.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalOperatorUtil.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalOperatorUtil.java
index a70d3f8..ba84429 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalOperatorUtil.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalOperatorUtil.java
@@ -17,6 +17,7 @@
  */
 package org.apache.drill.exec.physical.base;
 
+import com.google.common.base.Joiner;
 import com.google.common.collect.Lists;
 import org.apache.drill.common.config.CommonConstants;
 import org.apache.drill.common.config.DrillConfig;
@@ -27,17 +28,19 @@ import org.apache.drill.exec.proto.CoordinationProtos.DrillbitEndpoint;
 import java.util.List;
 
 public class PhysicalOperatorUtil {
-  static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(PhysicalOperatorUtil.class);
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(PhysicalOperatorUtil.class);
 
-  private PhysicalOperatorUtil(){}
 
-  public synchronized static Class<?>[] getSubTypes(DrillConfig config){
-    Class<?>[] ops =
+  private PhysicalOperatorUtil() {}
+
+  public synchronized static Class<?>[] getSubTypes(final DrillConfig config) {
+    final Class<?>[] ops =
         PathScanner.scanForImplementationsArr(PhysicalOperator.class,
             config.getStringList(CommonConstants.PHYSICAL_OPERATOR_SCAN_PACKAGES));
-    for (Class<?> op : ops) {
-      logger.debug("Adding physical operator {}", (Object) op );
-    }
+    final String lineBrokenList =
+        ops.length == 0 ? "" : "\n\t- " + Joiner.on("\n\t- ").join(ops);
+    logger.debug("Found {} physical operator classes: {}.", ops.length,
+                 lineBrokenList);
     return ops;
   }
 

http://git-wip-us.apache.org/repos/asf/drill/blob/5cd99162/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/OperatorCreatorRegistry.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/OperatorCreatorRegistry.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/OperatorCreatorRegistry.java
index 82a9a63..0be551b 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/OperatorCreatorRegistry.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/OperatorCreatorRegistry.java
@@ -35,8 +35,8 @@ public class OperatorCreatorRegistry {
   private volatile Map<Class<?>, Object> instanceRegistry = new HashMap<Class<?>,
Object>();
 
   public OperatorCreatorRegistry(DrillConfig config) {
-    addImplemntorsToMap(config, BatchCreator.class);
-    addImplemntorsToMap(config, RootCreator.class);
+    addImplementorsToMap(config, BatchCreator.class);
+    addImplementorsToMap(config, RootCreator.class);
     logger.debug("Adding Operator Creator map: {}", constructorRegistry);
   }
 
@@ -61,7 +61,7 @@ public class OperatorCreatorRegistry {
     }
   }
 
-  private <T> void addImplemntorsToMap(DrillConfig config, Class<T> baseInterface)
{
+  private <T> void addImplementorsToMap(DrillConfig config, Class<T> baseInterface)
{
     Class<?>[] providerClasses = PathScanner.scanForImplementationsArr(baseInterface,
         config.getStringList(CommonConstants.PHYSICAL_OPERATOR_SCAN_PACKAGES));
     for (Class<?> c : providerClasses) {

http://git-wip-us.apache.org/repos/asf/drill/blob/5cd99162/exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java
index 49a8a6b..d58f9a8 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java
@@ -60,6 +60,7 @@ import org.apache.drill.exec.store.sys.SystemTablePluginConfig;
 import org.apache.calcite.plan.RelOptRule;
 
 import com.google.common.base.Charsets;
+import com.google.common.base.Joiner;
 import com.google.common.base.Stopwatch;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSet.Builder;
@@ -106,11 +107,17 @@ public class StoragePluginRegistry implements Iterable<Map.Entry<String,
Storage
 
   @SuppressWarnings("unchecked")
   public void init() throws DrillbitStartupException {
-    DrillConfig config = context.getConfig();
-    Collection<Class<? extends StoragePlugin>> plugins =
-        PathScanner.scanForImplementations(StoragePlugin.class, config.getStringList(ExecConstants.STORAGE_ENGINE_SCAN_PACKAGES));
-    logger.debug("Loading storage plugins {}", plugins);
-    for (Class<? extends StoragePlugin> plugin : plugins) {
+    final DrillConfig config = context.getConfig();
+    final Collection<Class<? extends StoragePlugin>> pluginClasses =
+        PathScanner.scanForImplementations(
+            StoragePlugin.class,
+            config.getStringList(ExecConstants.STORAGE_ENGINE_SCAN_PACKAGES));
+    final String lineBrokenList =
+        pluginClasses.size() == 0
+        ? "" : "\n\t- " + Joiner.on("\n\t- ").join(pluginClasses);
+    logger.debug("Found {} storage plugin configuration classes: {}.",
+                 pluginClasses.size(), lineBrokenList);
+    for (Class<? extends StoragePlugin> plugin : pluginClasses) {
       int i = 0;
       for (Constructor<?> c : plugin.getConstructors()) {
         Class<?>[] params = c.getParameterTypes();


Mime
View raw message