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. :-(

common/src/main/java/org/apache/drill/common/config/DrillConfig.java

@@ -34,22 +34,21 @@
 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;
 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,18 +65,15 @@ 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) {
       SimpleModule deserModule = new SimpleModule("LogicalExpressionDeserializationModule")
         .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 List<String> getStartupArguments() {
 
   /**
    * 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 static DrillConfig forClient() {
    * @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 static DrillConfig create(Properties testConfigurations) {
   }
 
   /**
-   * ...
    * @param overrideFileResourcePathname
    *          see {@link #create(String)}'s {@code overrideFileResourcePathname}
    */
@@ -165,7 +161,6 @@ public static DrillConfig create(String overrideFileResourcePathname, boolean en
   }
 
   /**
-   * ...
    * @param overrideFileResourcePathname
    *          see {@link #create(String)}'s {@code overrideFileResourcePathname}
    * @param overriderProps
@@ -190,7 +185,7 @@ private static DrillConfig create(String overrideFileResourcePathname,
       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 @@ private static DrillConfig create(String overrideFileResourcePathname,
     }
 
     // 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);
     }

common/src/main/java/org/apache/drill/common/logical/FormatPluginConfigBase.java

@@ -23,8 +23,11 @@
 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;
   }
 

common/src/main/java/org/apache/drill/common/logical/StoragePluginConfigBase.java

@@ -23,20 +23,23 @@
 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;
   }
 

common/src/main/java/org/apache/drill/common/logical/data/LogicalOperatorBase.java

@@ -30,6 +30,8 @@
 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 void setMemo(String memo) {
     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;
   }
 }

common/src/main/java/org/apache/drill/common/util/PathScanner.java

@@ -37,14 +37,12 @@
 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,13 +79,19 @@ private static Reflections getReflections() {
     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);
     Class<?>[] arr = imps.toArray(new Class<?>[imps.size()]);
     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();

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 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;
   }
 

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 synchronized Object getOperatorCreator(Class<?> operator) throws Executio
     }
   }
 
-  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) {

exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java

@@ -60,6 +60,7 @@
 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 PStore<StoragePluginConfig> getStore() {
 
   @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();