You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by dsbos <gi...@git.apache.org> on 2015/07/15 02:08:50 UTC
[GitHub] drill pull request: Enhanced logging and associated code hygiene
GitHub user dsbos opened a pull request:
https://github.com/apache/drill/pull/93
Enhanced logging and associated code hygiene
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dsbos/incubator-drill bugs/drill-2815-etc-Logging
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/93.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #93
----
commit 24390a34e6886d019382f0e0c00cab570a499045
Author: dbarclay <db...@maprtech.com>
Date: 2015-07-14T04:02:40Z
DRILL-2815: Augment PathScanner logging; clean up code.
Added some DEBUG- and TRACE-level log calls.
Augmented/edited existing log call.
Misc. code hygiene:
- Added documentation of forResources(...).
- Renamed to clarify a number of local names.
- Made logger private.
- Added "final".
- Fixed indentation; wrapped some long lines; etc.
Conflicts:
common/src/main/java/org/apache/drill/common/util/PathScanner.java
commit e740ac4f7acc022922b5e5eebe209de42c504406
Author: dbarclay <db...@maprtech.com>
Date: 2015-07-14T19:11:36Z
DRILL-3496: Augment logging in DrillConfig and classpath scanning.
Main: Added and augmented logging calls.
Code Hygiene:
- Added/revised doc. comments, code comments.
- Renamed some locals.
- Added "final".
- Wrapped lines.
Also fixed currentTimeMillis -> Stopwatch in Drillbit.
Conflicts:
common/src/main/java/org/apache/drill/common/config/DrillConfig.java
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r34642779
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -44,26 +46,33 @@
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 org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(DrillConfig.class);
+ private static final Logger logger = getLogger(DrillConfig.class);
--- End diff --
To get rid of the unnecessarily verbose qualified names in that boilerplate code (which, when having any unnecessary verbosity, adds unnecessary risk of not noticing mistakes like this:
public class TestMultiInputAdd extends PopUnitTestBase {
// private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(TestMathFunctions.class);
)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35052392
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -90,7 +97,8 @@ public DrillConfig(Config config, boolean enableServer) {
}
/**
- * Create a DrillConfig object using the default config file name
+ * Creates a DrillConfig object using the default config file name
+ * and with server-specific configuration options disabled.
--- End diff --
Fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35268073
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +154,83 @@ public static DrillConfig create(Properties testConfigurations) {
return create(null, testConfigurations, true);
}
- public static DrillConfig create(String overrideFileName, boolean enableServerConfigs) {
- return create(overrideFileName, null, enableServerConfigs);
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ */
+ public static DrillConfig create(String overrideFileResourcePathname, boolean enableServerConfigs) {
+ return create(overrideFileResourcePathname, null, enableServerConfigs);
}
- private static DrillConfig create(String overrideFileName, Properties overriderProps, boolean enableServerConfigs) {
- overrideFileName = overrideFileName == null ? CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
- // first we load defaults.
+ /**
+ * ...
--- End diff --
I think if you should just put in explicit todo. However I'm not even sure that it makes sense to document these methods individually. There is a comment that is very descriptive about what all of these do, it's just above the wrong version of the method. If you move it above the one that takes all of the parameters, I think it would be fine to even leave out the parameter descriptions on the other versions and have a common message directing people to the one descriptive comment (which also describes all possible parameters). No need for a todo, not a bunch of work up front.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35041506
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -115,19 +124,26 @@ public static DrillConfig forClient() {
* <p>
* Configuration values are retrieved as follows:
* <ul>
- * <li>Check a single copy of "drill-override.conf". If multiple copies are on the classpath, behavior is
- * indeterminate. If a non-null value for overrideFileName is provided, this is utilized instead of drill-override.conf.</li>
- * <li>Check all copies of "drill-module.conf". Loading order is indeterminate.</li>
- * <li>Check a single copy of "drill-default.conf". If multiple copies are on the classpath, behavior is
- * indeterminate.</li>
+ * <li>Check a single copy of "drill-override.conf". If multiple copies are
+ * on the classpath, which copy is read is indeterminate.
+ * If a non-null value for overrideFileResourcePathname is provided, this
+ * is used instead of "{@code drill-override.conf}".</li>
+ * <li>Check all copies of "{@code drill-module.conf}". Loading order is
+ * indeterminate.</li>
+ * <li>Check a single copy of "{@code drill-default.conf}". If multiple
+ * copies are on the classpath, which copy is read is indeterminate.</li>
* </ul>
*
* </p>
- * @param overrideFileName The name of the file to use for override purposes.
+ * @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)
--- End diff --
Sorry for the nitpick, for clarity maybe use something like "<strong>not</strong> to disable looking for an.."
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on the pull request:
https://github.com/apache/drill/pull/93#issuecomment-125274722
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by Jim Scott <js...@maprtech.com>.
To make the topic even that much more annoying to discuss, a static final
variable in java is typically noted in all UPPER CASE...
I would vote for this notation and any of the following would be my
personal preference, but I am not a committer... soooo....
private static final Logger logger = LoggerFactory.
getLogger(DrillConfig.class);
private static final Logger LOG = LoggerFactory.
getLogger(DrillConfig.class);
private static final Logger LOGGER = LoggerFactory.
getLogger(DrillConfig.class);
On Mon, Jul 20, 2015 at 2:07 PM, jaltekruse <gi...@git.apache.org> wrote:
> Github user jaltekruse commented on a diff in the pull request:
>
> https://github.com/apache/drill/pull/93#discussion_r35031355
>
> --- Diff:
> common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
> @@ -44,26 +46,33 @@
> 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 org.slf4j.Logger logger =
> org.slf4j.LoggerFactory.getLogger(DrillConfig.class);
> + private static final Logger logger = getLogger(DrillConfig.class);
> --- End diff --
>
> I don't have a strong opinion about this either way, but I do think it
> is something we should just decide and make sure we have a hard consensus
> around the way it should be done. I would like to see this change made in
> bulk if we decide to do it. Please do as Chris did with his proposal to
> make the loggers private and suggest the change on the mailing list. If we
> can come to a consensus that we want to change it, we can open up a JIRA at
> that time to make a bulk replacement. Or as we have done with the issue
> around privatizing them, we can refactor over time as people see the wrong
> convention in a file they are editing.
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
--
*Jim Scott*
Director, Enterprise Strategy & Architecture
+1 (347) 746-9281
<http://www.mapr.com/>
[image: MapR Technologies] <http://www.mapr.com>
Now Available - Free Hadoop On-Demand Training
<http://www.mapr.com/training?utm_source=Email&utm_medium=Signature&utm_campaign=Free%20available>
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35031355
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -44,26 +46,33 @@
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 org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(DrillConfig.class);
+ private static final Logger logger = getLogger(DrillConfig.class);
--- End diff --
I don't have a strong opinion about this either way, but I do think it is something we should just decide and make sure we have a hard consensus around the way it should be done. I would like to see this change made in bulk if we decide to do it. Please do as Chris did with his proposal to make the loggers private and suggest the change on the mailing list. If we can come to a consensus that we want to change it, we can open up a JIRA at that time to make a bulk replacement. Or as we have done with the issue around privatizing them, we can refactor over time as people see the wrong convention in a file they are editing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on the pull request:
https://github.com/apache/drill/pull/93#issuecomment-125272213
(More "final" added in already touched methods.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r34634834
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -44,26 +46,33 @@
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 org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(DrillConfig.class);
+ private static final Logger logger = getLogger(DrillConfig.class);
--- End diff --
Why not *just* uncomment (the pattern we follow everywhere)? Why add imports?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r34650500
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +155,77 @@ public static DrillConfig create(Properties testConfigurations) {
return create(null, testConfigurations, true);
}
- public static DrillConfig create(String overrideFileName, boolean enableServerConfigs) {
- return create(overrideFileName, null, enableServerConfigs);
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ */
+ public static DrillConfig create(String overrideFileResourcePathname, boolean enableServerConfigs) {
+ return create(overrideFileResourcePathname, null, enableServerConfigs);
}
- private static DrillConfig create(String overrideFileName, Properties overriderProps, boolean enableServerConfigs) {
- overrideFileName = overrideFileName == null ? CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
- // first we load defaults.
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ * @param overriderProps
+ * optional property map for further overriding (after override file
+ * is assimilated
+ * @param enableServerConfigs
+ * whether to enable server-specific configuration options
+ * @return
+ */
+ private static DrillConfig create(String overrideFileResourcePathname,
+ final Properties overriderProps,
+ final boolean enableServerConfigs) {
+ overrideFileResourcePathname =
+ overrideFileResourcePathname == null
+ ? CommonConstants.CONFIG_OVERRIDE
+ : overrideFileResourcePathname;
+
+ // 1. Load defaults configuration file.
Config fallback = null;
final ClassLoader[] classLoaders = ClasspathHelper.classLoaders();
for (ClassLoader classLoader : classLoaders) {
- if (classLoader.getResource(CommonConstants.CONFIG_DEFAULT) != null) {
- fallback = ConfigFactory.load(classLoader, CommonConstants.CONFIG_DEFAULT);
+ final URL url =
+ classLoader.getResource(CommonConstants.CONFIG_DEFAULT);
+ if (null != url) {
+ logger.debug("Loading base config. file at {}.", url);
+ fallback =
+ ConfigFactory.load(classLoader,
+ CommonConstants.CONFIG_DEFAULT);
break;
}
}
+ // 2. Load per-module configuration files.
Collection<URL> urls = PathScanner.getConfigURLs();
logger.debug("Loading configs at the following URLs {}", urls);
for (URL url : urls) {
+ logger.debug("Loading module config. file at {}.", url);
fallback = ConfigFactory.parseURL(url).withFallback(fallback);
}
- Config effectiveConfig = ConfigFactory.load(overrideFileName).withFallback(fallback);
+ // 3. Load any overrides configuration file and overrides from JVM
--- End diff --
That "overrides" is part of the noun phrase "overrides configuration file" (parallel to "defaults configuration file" and "per-module configuration file") (not a reference to the overrides from any such file).
Yes, it's a bit ambiguous between meaning that and seeming like a word is missing if one doesn't see the other phrases. (Changing the "any" to "the" might reduce that ambiguity, but then it would imply that that code expects there to be an overrides configuration file, when it's actually optional.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos closed the pull request at:
https://github.com/apache/drill/pull/93
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35136041
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +154,76 @@ public static DrillConfig create(Properties testConfigurations) {
return create(null, testConfigurations, true);
}
- public static DrillConfig create(String overrideFileName, boolean enableServerConfigs) {
- return create(overrideFileName, null, enableServerConfigs);
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ */
+ public static DrillConfig create(String overrideFileResourcePathname, boolean enableServerConfigs) {
+ return create(overrideFileResourcePathname, null, enableServerConfigs);
}
- private static DrillConfig create(String overrideFileName, Properties overriderProps, boolean enableServerConfigs) {
- overrideFileName = overrideFileName == null ? CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
- // first we load defaults.
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ * @param overriderProps
+ * optional property map for further overriding (after override file
+ * is assimilated
+ * @param enableServerConfigs
+ * whether to enable server-specific configuration options
+ * @return
+ */
+ private static DrillConfig create(String overrideFileResourcePathname,
+ final Properties overriderProps,
+ final boolean enableServerConfigs) {
+ overrideFileResourcePathname =
+ overrideFileResourcePathname == null
+ ? CommonConstants.CONFIG_OVERRIDE
+ : overrideFileResourcePathname;
+
+ // 1. Load defaults configuration file.
Config fallback = null;
final ClassLoader[] classLoaders = ClasspathHelper.classLoaders();
for (ClassLoader classLoader : classLoaders) {
- if (classLoader.getResource(CommonConstants.CONFIG_DEFAULT) != null) {
- fallback = ConfigFactory.load(classLoader, CommonConstants.CONFIG_DEFAULT);
+ final URL url =
+ classLoader.getResource(CommonConstants.CONFIG_DEFAULT);
+ if (null != url) {
+ logger.debug("Loading base configuration file at {}.", url);
+ fallback =
+ ConfigFactory.load(classLoader,
+ CommonConstants.CONFIG_DEFAULT);
break;
}
}
+ // 2. Load per-module configuration files.
Collection<URL> urls = PathScanner.getConfigURLs();
- logger.debug("Loading configs at the following URLs {}", urls);
for (URL url : urls) {
+ logger.debug("Loading module configuration file at {}.", url);
--- End diff --
Done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35032132
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -90,7 +97,8 @@ public DrillConfig(Config config, boolean enableServer) {
}
/**
- * Create a DrillConfig object using the default config file name
+ * Creates a DrillConfig object using the default config file name
+ * and with server-specific configuration options disabled.
--- End diff --
I am pretty sure this us supposed to say "enabled"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35053589
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -115,19 +124,26 @@ public static DrillConfig forClient() {
* <p>
* Configuration values are retrieved as follows:
* <ul>
- * <li>Check a single copy of "drill-override.conf". If multiple copies are on the classpath, behavior is
- * indeterminate. If a non-null value for overrideFileName is provided, this is utilized instead of drill-override.conf.</li>
- * <li>Check all copies of "drill-module.conf". Loading order is indeterminate.</li>
- * <li>Check a single copy of "drill-default.conf". If multiple copies are on the classpath, behavior is
- * indeterminate.</li>
+ * <li>Check a single copy of "drill-override.conf". If multiple copies are
+ * on the classpath, which copy is read is indeterminate.
+ * If a non-null value for overrideFileResourcePathname is provided, this
+ * is used instead of "{@code drill-override.conf}".</li>
+ * <li>Check all copies of "{@code drill-module.conf}". Loading order is
+ * indeterminate.</li>
+ * <li>Check a single copy of "{@code drill-default.conf}". If multiple
+ * copies are on the classpath, which copy is read is indeterminate.</li>
* </ul>
*
* </p>
- * @param overrideFileName The name of the file to use for override purposes.
+ * @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)
--- End diff --
Reworded.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on the pull request:
https://github.com/apache/drill/pull/93#issuecomment-121513460
> ... clutter Lilith ... 3) Every other log statement in your patch should be TRACE level. ...
Commit 872309a... reverts to one log call and log entry per list, but retains the improved readability by putting each listed item on a separate line.
> ... 1) Overrides in DrillConfig (step 3 and 4 in DrillConfig#create) could be at INFO level. ...
Since the set of per-module configuration files is affected by user action (installing plug-ins), step 2's logging should probably be at the same level.
And then, since that's reporting every configuration file except the defaults file, maybe the defaults file should also be listed at the same level.
Comments? (INFO for all those config files?)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r34708277
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +154,76 @@ public static DrillConfig create(Properties testConfigurations) {
return create(null, testConfigurations, true);
}
- public static DrillConfig create(String overrideFileName, boolean enableServerConfigs) {
- return create(overrideFileName, null, enableServerConfigs);
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ */
+ public static DrillConfig create(String overrideFileResourcePathname, boolean enableServerConfigs) {
+ return create(overrideFileResourcePathname, null, enableServerConfigs);
}
- private static DrillConfig create(String overrideFileName, Properties overriderProps, boolean enableServerConfigs) {
- overrideFileName = overrideFileName == null ? CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
- // first we load defaults.
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ * @param overriderProps
+ * optional property map for further overriding (after override file
+ * is assimilated
+ * @param enableServerConfigs
+ * whether to enable server-specific configuration options
+ * @return
+ */
+ private static DrillConfig create(String overrideFileResourcePathname,
+ final Properties overriderProps,
+ final boolean enableServerConfigs) {
+ overrideFileResourcePathname =
+ overrideFileResourcePathname == null
+ ? CommonConstants.CONFIG_OVERRIDE
+ : overrideFileResourcePathname;
+
+ // 1. Load defaults configuration file.
Config fallback = null;
final ClassLoader[] classLoaders = ClasspathHelper.classLoaders();
for (ClassLoader classLoader : classLoaders) {
- if (classLoader.getResource(CommonConstants.CONFIG_DEFAULT) != null) {
- fallback = ConfigFactory.load(classLoader, CommonConstants.CONFIG_DEFAULT);
+ final URL url =
+ classLoader.getResource(CommonConstants.CONFIG_DEFAULT);
+ if (null != url) {
+ logger.debug("Loading base configuration file at {}.", url);
+ fallback =
+ ConfigFactory.load(classLoader,
+ CommonConstants.CONFIG_DEFAULT);
break;
}
}
+ // 2. Load per-module configuration files.
Collection<URL> urls = PathScanner.getConfigURLs();
- logger.debug("Loading configs at the following URLs {}", urls);
for (URL url : urls) {
+ logger.debug("Loading module configuration file at {}.", url);
--- End diff --
Could you squash these into one log message too?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35136191
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -44,31 +46,37 @@
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 org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(DrillConfig.class);
+ private static final Logger logger = getLogger(DrillConfig.class);
+
private final ObjectMapper mapper;
private final ImmutableList<String> startupArguments;
public static final boolean ON_OSX = System.getProperty("os.name").contains("OS X");
- @SuppressWarnings("restriction") private static final long MAX_DIRECT_MEMORY = sun.misc.VM.maxDirectMemory();
+ @SuppressWarnings("restriction")
+ private static final long MAX_DIRECT_MEMORY = sun.misc.VM.maxDirectMemory();
@SuppressWarnings("unchecked")
private volatile List<Queue<Object>> sinkQueues = new CopyOnWriteArrayList<Queue<Object>>(new Queue[1]);
@VisibleForTesting
- public DrillConfig(Config config, boolean enableServer) {
+ public DrillConfig(Config config, boolean enableServerConfigs) {
super(config);
- logger.debug("Setting up config object.");
+ logger.debug("Setting up DrillConfig object.");
+ if (logger.isTraceEnabled()) {
--- End diff --
Removed isTraceEnabled guard.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35048317
--- Diff: common/src/main/java/org/apache/drill/common/logical/FormatPluginConfigBase.java ---
@@ -27,11 +27,20 @@
static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FormatPluginConfigBase.class);
- public synchronized static Class<?>[] getSubTypes(DrillConfig config){
- List<String> packages = config.getStringList(CommonConstants.STORAGE_PLUGIN_CONFIG_SCAN_PACKAGES);
- Class<?>[] sec = PathScanner.scanForImplementationsArr(FormatPluginConfig.class, packages);
- logger.debug("Adding Format Plugin Configs including {}", (Object) sec );
- return sec;
+ public synchronized static Class<?>[] getSubTypes(DrillConfig config) {
+ List<String> packages =
+ config.getStringList(CommonConstants.STORAGE_PLUGIN_CONFIG_SCAN_PACKAGES);
+ Class<?>[] pluginClasses =
+ PathScanner.scanForImplementationsArr(FormatPluginConfig.class, packages);
+ if (logger.isDebugEnabled()) {
+ final StringBuilder sb = new StringBuilder();
--- End diff --
I don't have a strong recommendation for where it would belong, but as long as this code is in 3 places in this patch, and it seems like it would be useful for other cases, we should probably make it available in a static utility class.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35042277
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +154,83 @@ public static DrillConfig create(Properties testConfigurations) {
return create(null, testConfigurations, true);
}
- public static DrillConfig create(String overrideFileName, boolean enableServerConfigs) {
- return create(overrideFileName, null, enableServerConfigs);
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ */
+ public static DrillConfig create(String overrideFileResourcePathname, boolean enableServerConfigs) {
+ return create(overrideFileResourcePathname, null, enableServerConfigs);
}
- private static DrillConfig create(String overrideFileName, Properties overriderProps, boolean enableServerConfigs) {
- overrideFileName = overrideFileName == null ? CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
- // first we load defaults.
+ /**
+ * ...
--- End diff --
I haven't seen this convention before, but I assume this means that the docs for similar methods suffice and don't need to be repeated here. I would possibly recommend moving the longer comment above to here, in which case I think it would be possible to just put "..." above all of the alternative implementations of this method. I think it makes the most sense to put the comment describing the general idea of the functions purpose above the version that also describes what all of parameters can do.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/93#issuecomment-121452663
It seems we will be logging too much at DEBUG level (mainly through LogicalOperatorBase and PhysicalOperatorUtil); it will clutter Lilith specially.
IMO,
1) Overrides in DrillConfig (step 3 and 4 in DrillConfig#create) could be at INFO level.
2) Drillbit logs could be as they are.
3) Every other log statement in your patch should be TRACE level.
What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35031778
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -44,31 +46,37 @@
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 org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(DrillConfig.class);
+ private static final Logger logger = getLogger(DrillConfig.class);
+
private final ObjectMapper mapper;
private final ImmutableList<String> startupArguments;
public static final boolean ON_OSX = System.getProperty("os.name").contains("OS X");
- @SuppressWarnings("restriction") private static final long MAX_DIRECT_MEMORY = sun.misc.VM.maxDirectMemory();
+ @SuppressWarnings("restriction")
+ private static final long MAX_DIRECT_MEMORY = sun.misc.VM.maxDirectMemory();
@SuppressWarnings("unchecked")
private volatile List<Queue<Object>> sinkQueues = new CopyOnWriteArrayList<Queue<Object>>(new Queue[1]);
@VisibleForTesting
- public DrillConfig(Config config, boolean enableServer) {
+ public DrillConfig(Config config, boolean enableServerConfigs) {
super(config);
- logger.debug("Setting up config object.");
+ logger.debug("Setting up DrillConfig object.");
+ if (logger.isTraceEnabled()) {
--- End diff --
I understand that this is to avoid the call the render() function if we don't need to log the statement, but isn't reading config file pretty rare to require this kind of optimization here? Did you notice a slow down on the drillbit startup time if we were always rendering the options, even if we were not logging them?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35053195
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -44,31 +46,37 @@
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 org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(DrillConfig.class);
+ private static final Logger logger = getLogger(DrillConfig.class);
+
private final ObjectMapper mapper;
private final ImmutableList<String> startupArguments;
public static final boolean ON_OSX = System.getProperty("os.name").contains("OS X");
- @SuppressWarnings("restriction") private static final long MAX_DIRECT_MEMORY = sun.misc.VM.maxDirectMemory();
+ @SuppressWarnings("restriction")
+ private static final long MAX_DIRECT_MEMORY = sun.misc.VM.maxDirectMemory();
@SuppressWarnings("unchecked")
private volatile List<Queue<Object>> sinkQueues = new CopyOnWriteArrayList<Queue<Object>>(new Queue[1]);
@VisibleForTesting
- public DrillConfig(Config config, boolean enableServer) {
+ public DrillConfig(Config config, boolean enableServerConfigs) {
super(config);
- logger.debug("Setting up config object.");
+ logger.debug("Setting up DrillConfig object.");
+ if (logger.isTraceEnabled()) {
--- End diff --
No, I didn't expect it to take any significant time; I just thought someone would object if the calls weren't guarded by that if statement.
So ... remove it to reduce the clutter, or keep/leave it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35126587
--- Diff: common/src/main/java/org/apache/drill/common/logical/FormatPluginConfigBase.java ---
@@ -27,11 +27,20 @@
static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FormatPluginConfigBase.class);
- public synchronized static Class<?>[] getSubTypes(DrillConfig config){
- List<String> packages = config.getStringList(CommonConstants.STORAGE_PLUGIN_CONFIG_SCAN_PACKAGES);
- Class<?>[] sec = PathScanner.scanForImplementationsArr(FormatPluginConfig.class, packages);
- logger.debug("Adding Format Plugin Configs including {}", (Object) sec );
- return sec;
+ public synchronized static Class<?>[] getSubTypes(DrillConfig config) {
+ List<String> packages =
+ config.getStringList(CommonConstants.STORAGE_PLUGIN_CONFIG_SCAN_PACKAGES);
+ Class<?>[] pluginClasses =
+ PathScanner.scanForImplementationsArr(FormatPluginConfig.class, packages);
+ if (logger.isDebugEnabled()) {
+ final StringBuilder sb = new StringBuilder();
--- End diff --
There believe there are at least packages in common that claim to hold general utilities. If you can't find a class to put in a utility function, I think it would be fine to create one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r34639451
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +155,77 @@ public static DrillConfig create(Properties testConfigurations) {
return create(null, testConfigurations, true);
}
- public static DrillConfig create(String overrideFileName, boolean enableServerConfigs) {
- return create(overrideFileName, null, enableServerConfigs);
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ */
+ public static DrillConfig create(String overrideFileResourcePathname, boolean enableServerConfigs) {
+ return create(overrideFileResourcePathname, null, enableServerConfigs);
}
- private static DrillConfig create(String overrideFileName, Properties overriderProps, boolean enableServerConfigs) {
- overrideFileName = overrideFileName == null ? CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
- // first we load defaults.
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ * @param overriderProps
+ * optional property map for further overriding (after override file
+ * is assimilated
+ * @param enableServerConfigs
+ * whether to enable server-specific configuration options
+ * @return
+ */
+ private static DrillConfig create(String overrideFileResourcePathname,
+ final Properties overriderProps,
+ final boolean enableServerConfigs) {
+ overrideFileResourcePathname =
+ overrideFileResourcePathname == null
+ ? CommonConstants.CONFIG_OVERRIDE
+ : overrideFileResourcePathname;
+
+ // 1. Load defaults configuration file.
Config fallback = null;
final ClassLoader[] classLoaders = ClasspathHelper.classLoaders();
for (ClassLoader classLoader : classLoaders) {
- if (classLoader.getResource(CommonConstants.CONFIG_DEFAULT) != null) {
- fallback = ConfigFactory.load(classLoader, CommonConstants.CONFIG_DEFAULT);
+ final URL url =
+ classLoader.getResource(CommonConstants.CONFIG_DEFAULT);
+ if (null != url) {
+ logger.debug("Loading base config. file at {}.", url);
+ fallback =
+ ConfigFactory.load(classLoader,
+ CommonConstants.CONFIG_DEFAULT);
break;
}
}
+ // 2. Load per-module configuration files.
Collection<URL> urls = PathScanner.getConfigURLs();
logger.debug("Loading configs at the following URLs {}", urls);
for (URL url : urls) {
+ logger.debug("Loading module config. file at {}.", url);
fallback = ConfigFactory.parseURL(url).withFallback(fallback);
}
- Config effectiveConfig = ConfigFactory.load(overrideFileName).withFallback(fallback);
+ // 3. Load any overrides configuration file and overrides from JVM
--- End diff --
overrides *from* configuration file
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on the pull request:
https://github.com/apache/drill/pull/93#issuecomment-121437274
DRILL-2815, DRILL-3496 Adding a link to the related JIRA reports
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/93#issuecomment-123410347
In 4e9a2a1 commit message, you mentioned that you added "final". There are quite a few places where you have not added "final". Please fix?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35133290
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -44,31 +46,37 @@
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 org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(DrillConfig.class);
+ private static final Logger logger = getLogger(DrillConfig.class);
+
private final ObjectMapper mapper;
private final ImmutableList<String> startupArguments;
public static final boolean ON_OSX = System.getProperty("os.name").contains("OS X");
- @SuppressWarnings("restriction") private static final long MAX_DIRECT_MEMORY = sun.misc.VM.maxDirectMemory();
+ @SuppressWarnings("restriction")
+ private static final long MAX_DIRECT_MEMORY = sun.misc.VM.maxDirectMemory();
@SuppressWarnings("unchecked")
private volatile List<Queue<Object>> sinkQueues = new CopyOnWriteArrayList<Queue<Object>>(new Queue[1]);
@VisibleForTesting
- public DrillConfig(Config config, boolean enableServer) {
+ public DrillConfig(Config config, boolean enableServerConfigs) {
super(config);
- logger.debug("Setting up config object.");
+ logger.debug("Setting up DrillConfig object.");
+ if (logger.isTraceEnabled()) {
--- End diff --
Again I'm not particularly opinionated on this one. If we are trying to establish best practices, I would argue for leaving out the guard unless you are confident enough about the need for it that you would put in a comment describing the overhead you are avoiding. Otherwise we will have inconsistent patterns, and someone might find them and assume we always want the guards.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r34649690
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +155,77 @@ public static DrillConfig create(Properties testConfigurations) {
return create(null, testConfigurations, true);
}
- public static DrillConfig create(String overrideFileName, boolean enableServerConfigs) {
- return create(overrideFileName, null, enableServerConfigs);
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ */
+ public static DrillConfig create(String overrideFileResourcePathname, boolean enableServerConfigs) {
+ return create(overrideFileResourcePathname, null, enableServerConfigs);
}
- private static DrillConfig create(String overrideFileName, Properties overriderProps, boolean enableServerConfigs) {
- overrideFileName = overrideFileName == null ? CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
- // first we load defaults.
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ * @param overriderProps
+ * optional property map for further overriding (after override file
+ * is assimilated
+ * @param enableServerConfigs
+ * whether to enable server-specific configuration options
+ * @return
+ */
+ private static DrillConfig create(String overrideFileResourcePathname,
+ final Properties overriderProps,
+ final boolean enableServerConfigs) {
+ overrideFileResourcePathname =
+ overrideFileResourcePathname == null
+ ? CommonConstants.CONFIG_OVERRIDE
+ : overrideFileResourcePathname;
+
+ // 1. Load defaults configuration file.
Config fallback = null;
final ClassLoader[] classLoaders = ClasspathHelper.classLoaders();
for (ClassLoader classLoader : classLoaders) {
- if (classLoader.getResource(CommonConstants.CONFIG_DEFAULT) != null) {
- fallback = ConfigFactory.load(classLoader, CommonConstants.CONFIG_DEFAULT);
+ final URL url =
+ classLoader.getResource(CommonConstants.CONFIG_DEFAULT);
+ if (null != url) {
+ logger.debug("Loading base config. file at {}.", url);
+ fallback =
+ ConfigFactory.load(classLoader,
+ CommonConstants.CONFIG_DEFAULT);
break;
}
}
+ // 2. Load per-module configuration files.
Collection<URL> urls = PathScanner.getConfigURLs();
logger.debug("Loading configs at the following URLs {}", urls);
for (URL url : urls) {
+ logger.debug("Loading module config. file at {}.", url);
--- End diff --
First call removed in update.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35054916
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +154,83 @@ public static DrillConfig create(Properties testConfigurations) {
return create(null, testConfigurations, true);
}
- public static DrillConfig create(String overrideFileName, boolean enableServerConfigs) {
- return create(overrideFileName, null, enableServerConfigs);
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ */
+ public static DrillConfig create(String overrideFileResourcePathname, boolean enableServerConfigs) {
+ return create(overrideFileResourcePathname, null, enableServerConfigs);
}
- private static DrillConfig create(String overrideFileName, Properties overriderProps, boolean enableServerConfigs) {
- overrideFileName = overrideFileName == null ? CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
- // first we load defaults.
+ /**
+ * ...
--- End diff --
[Reply via e-mail because I don't currently see this comment of yours on the pull-request page.]
> > + /**
> > + * ...xx
> I haven't seen this convention before, but I assume this means that the docs for similar methods suffice and don't need to be repeated here.
No, actually it was an attempt to indicate that I hadn't addressed the method description (the part before the parameters), and not intended to mean "otherwise same as above".
I was trying to quickly at least document the parameter (add the reference to the other existing text) without taking time to fill in the whole documentation comment.
Would simply leaving the method description part blank be better than using "..."?
Daniel
Jason Altekruse wrote:
>
> In common/src/main/java/org/apache/drill/common/config/DrillConfig.java <https://github.com/apache/drill/pull/93#discussion_r35042277>:
>
> > }
> >
> > - private static DrillConfig create(String overrideFileName, Properties overriderProps, boolean enableServerConfigs) {
> > - overrideFileName = overrideFileName == null ? CommonConstants.CONFIG_OVERRIDE : overrideFileName;
> > -
> > - // first we load defaults.
> > + /**
> > + * ...
>
> I haven't seen this convention before, but I assume this means that the docs for similar methods suffice and don't need to be repeated here. I would possibly recommend moving the longer comment above to here, in which case I think it would be possible to just put "..." above all of the alternative implementations of this method. I think it makes the most sense to put the comment describing the general idea of the functions purpose above the version that also describes what all of parameters can do.
>
> —
> Reply to this email directly or view it on GitHub <https://github.com/apache/drill/pull/93/files#r35042277>.
>
--
Daniel Barclay
MapR Technologies
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35052980
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +155,77 @@ public static DrillConfig create(Properties testConfigurations) {
return create(null, testConfigurations, true);
}
- public static DrillConfig create(String overrideFileName, boolean enableServerConfigs) {
- return create(overrideFileName, null, enableServerConfigs);
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ */
+ public static DrillConfig create(String overrideFileResourcePathname, boolean enableServerConfigs) {
+ return create(overrideFileResourcePathname, null, enableServerConfigs);
}
- private static DrillConfig create(String overrideFileName, Properties overriderProps, boolean enableServerConfigs) {
- overrideFileName = overrideFileName == null ? CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
- // first we load defaults.
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ * @param overriderProps
+ * optional property map for further overriding (after override file
+ * is assimilated
+ * @param enableServerConfigs
+ * whether to enable server-specific configuration options
+ * @return
+ */
+ private static DrillConfig create(String overrideFileResourcePathname,
+ final Properties overriderProps,
+ final boolean enableServerConfigs) {
+ overrideFileResourcePathname =
+ overrideFileResourcePathname == null
+ ? CommonConstants.CONFIG_OVERRIDE
+ : overrideFileResourcePathname;
+
+ // 1. Load defaults configuration file.
Config fallback = null;
final ClassLoader[] classLoaders = ClasspathHelper.classLoaders();
for (ClassLoader classLoader : classLoaders) {
- if (classLoader.getResource(CommonConstants.CONFIG_DEFAULT) != null) {
- fallback = ConfigFactory.load(classLoader, CommonConstants.CONFIG_DEFAULT);
+ final URL url =
+ classLoader.getResource(CommonConstants.CONFIG_DEFAULT);
+ if (null != url) {
+ logger.debug("Loading base config. file at {}.", url);
+ fallback =
+ ConfigFactory.load(classLoader,
+ CommonConstants.CONFIG_DEFAULT);
break;
}
}
+ // 2. Load per-module configuration files.
Collection<URL> urls = PathScanner.getConfigURLs();
logger.debug("Loading configs at the following URLs {}", urls);
for (URL url : urls) {
+ logger.debug("Loading module config. file at {}.", url);
fallback = ConfigFactory.parseURL(url).withFallback(fallback);
}
- Config effectiveConfig = ConfigFactory.load(overrideFileName).withFallback(fallback);
+ // 3. Load any overrides configuration file and overrides from JVM
--- End diff --
Reworded a bit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35279254
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +154,83 @@ public static DrillConfig create(Properties testConfigurations) {
return create(null, testConfigurations, true);
}
- public static DrillConfig create(String overrideFileName, boolean enableServerConfigs) {
- return create(overrideFileName, null, enableServerConfigs);
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ */
+ public static DrillConfig create(String overrideFileResourcePathname, boolean enableServerConfigs) {
+ return create(overrideFileResourcePathname, null, enableServerConfigs);
}
- private static DrillConfig create(String overrideFileName, Properties overriderProps, boolean enableServerConfigs) {
- overrideFileName = overrideFileName == null ? CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
- // first we load defaults.
+ /**
+ * ...
--- End diff --
> I'm not even sure that it makes sense to document these methods individually.
If a method is not documented individually, then viewing that method's documentation isn't going to yield much information--remember that people don't see comments just in the context of the surrounding source code, but also in IDEs (e.g., showing one method's documentation at a time) and in generated Javadoc pages.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35053796
--- Diff: common/src/main/java/org/apache/drill/common/logical/FormatPluginConfigBase.java ---
@@ -27,11 +27,20 @@
static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FormatPluginConfigBase.class);
- public synchronized static Class<?>[] getSubTypes(DrillConfig config){
- List<String> packages = config.getStringList(CommonConstants.STORAGE_PLUGIN_CONFIG_SCAN_PACKAGES);
- Class<?>[] sec = PathScanner.scanForImplementationsArr(FormatPluginConfig.class, packages);
- logger.debug("Adding Format Plugin Configs including {}", (Object) sec );
- return sec;
+ public synchronized static Class<?>[] getSubTypes(DrillConfig config) {
+ List<String> packages =
+ config.getStringList(CommonConstants.STORAGE_PLUGIN_CONFIG_SCAN_PACKAGES);
+ Class<?>[] pluginClasses =
+ PathScanner.scanForImplementationsArr(FormatPluginConfig.class, packages);
+ if (logger.isDebugEnabled()) {
+ final StringBuilder sb = new StringBuilder();
--- End diff --
Yeah, I thought it should be factored out, but it wasn't clear where. (Also, the functionality is close to that of some utility join(...) methods--but not quite the same.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r34634944
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +155,77 @@ public static DrillConfig create(Properties testConfigurations) {
return create(null, testConfigurations, true);
}
- public static DrillConfig create(String overrideFileName, boolean enableServerConfigs) {
- return create(overrideFileName, null, enableServerConfigs);
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ */
+ public static DrillConfig create(String overrideFileResourcePathname, boolean enableServerConfigs) {
+ return create(overrideFileResourcePathname, null, enableServerConfigs);
}
- private static DrillConfig create(String overrideFileName, Properties overriderProps, boolean enableServerConfigs) {
- overrideFileName = overrideFileName == null ? CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
- // first we load defaults.
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ * @param overriderProps
+ * optional property map for further overriding (after override file
+ * is assimilated
+ * @param enableServerConfigs
+ * whether to enable server-specific configuration options
+ * @return
+ */
+ private static DrillConfig create(String overrideFileResourcePathname,
+ final Properties overriderProps,
+ final boolean enableServerConfigs) {
+ overrideFileResourcePathname =
+ overrideFileResourcePathname == null
+ ? CommonConstants.CONFIG_OVERRIDE
+ : overrideFileResourcePathname;
+
+ // 1. Load defaults configuration file.
Config fallback = null;
final ClassLoader[] classLoaders = ClasspathHelper.classLoaders();
for (ClassLoader classLoader : classLoaders) {
- if (classLoader.getResource(CommonConstants.CONFIG_DEFAULT) != null) {
- fallback = ConfigFactory.load(classLoader, CommonConstants.CONFIG_DEFAULT);
+ final URL url =
+ classLoader.getResource(CommonConstants.CONFIG_DEFAULT);
+ if (null != url) {
+ logger.debug("Loading base config. file at {}.", url);
+ fallback =
+ ConfigFactory.load(classLoader,
+ CommonConstants.CONFIG_DEFAULT);
break;
}
}
+ // 2. Load per-module configuration files.
Collection<URL> urls = PathScanner.getConfigURLs();
logger.debug("Loading configs at the following URLs {}", urls);
for (URL url : urls) {
+ logger.debug("Loading module config. file at {}.", url);
--- End diff --
This is already printed from the above debug log?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/93#issuecomment-121695453
Since "user action" affects the messages, I'd say INFO.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Enhanced logging and associated code hygiene
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35042822
--- Diff: common/src/main/java/org/apache/drill/common/config/DrillConfig.java ---
@@ -138,34 +155,77 @@ public static DrillConfig create(Properties testConfigurations) {
return create(null, testConfigurations, true);
}
- public static DrillConfig create(String overrideFileName, boolean enableServerConfigs) {
- return create(overrideFileName, null, enableServerConfigs);
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ */
+ public static DrillConfig create(String overrideFileResourcePathname, boolean enableServerConfigs) {
+ return create(overrideFileResourcePathname, null, enableServerConfigs);
}
- private static DrillConfig create(String overrideFileName, Properties overriderProps, boolean enableServerConfigs) {
- overrideFileName = overrideFileName == null ? CommonConstants.CONFIG_OVERRIDE : overrideFileName;
-
- // first we load defaults.
+ /**
+ * ...
+ * @param overrideFileResourcePathname
+ * see {@link #create(String)}'s {@code overrideFileResourcePathname}
+ * @param overriderProps
+ * optional property map for further overriding (after override file
+ * is assimilated
+ * @param enableServerConfigs
+ * whether to enable server-specific configuration options
+ * @return
+ */
+ private static DrillConfig create(String overrideFileResourcePathname,
+ final Properties overriderProps,
+ final boolean enableServerConfigs) {
+ overrideFileResourcePathname =
+ overrideFileResourcePathname == null
+ ? CommonConstants.CONFIG_OVERRIDE
+ : overrideFileResourcePathname;
+
+ // 1. Load defaults configuration file.
Config fallback = null;
final ClassLoader[] classLoaders = ClasspathHelper.classLoaders();
for (ClassLoader classLoader : classLoaders) {
- if (classLoader.getResource(CommonConstants.CONFIG_DEFAULT) != null) {
- fallback = ConfigFactory.load(classLoader, CommonConstants.CONFIG_DEFAULT);
+ final URL url =
+ classLoader.getResource(CommonConstants.CONFIG_DEFAULT);
+ if (null != url) {
+ logger.debug("Loading base config. file at {}.", url);
+ fallback =
+ ConfigFactory.load(classLoader,
+ CommonConstants.CONFIG_DEFAULT);
break;
}
}
+ // 2. Load per-module configuration files.
Collection<URL> urls = PathScanner.getConfigURLs();
logger.debug("Loading configs at the following URLs {}", urls);
for (URL url : urls) {
+ logger.debug("Loading module config. file at {}.", url);
fallback = ConfigFactory.parseURL(url).withFallback(fallback);
}
- Config effectiveConfig = ConfigFactory.load(overrideFileName).withFallback(fallback);
+ // 3. Load any overrides configuration file and overrides from JVM
--- End diff --
Maybe "Load any available overrides configuration files..."
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-2818, DRILL-3496: Enhanced logging and a...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/93#discussion_r35559349
--- Diff: common/src/main/java/org/apache/drill/common/logical/FormatPluginConfigBase.java ---
@@ -27,11 +27,20 @@
static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FormatPluginConfigBase.class);
- public synchronized static Class<?>[] getSubTypes(DrillConfig config){
- List<String> packages = config.getStringList(CommonConstants.STORAGE_PLUGIN_CONFIG_SCAN_PACKAGES);
- Class<?>[] sec = PathScanner.scanForImplementationsArr(FormatPluginConfig.class, packages);
- logger.debug("Adding Format Plugin Configs including {}", (Object) sec );
- return sec;
+ public synchronized static Class<?>[] getSubTypes(DrillConfig config) {
+ List<String> packages =
+ config.getStringList(CommonConstants.STORAGE_PLUGIN_CONFIG_SCAN_PACKAGES);
+ Class<?>[] pluginClasses =
+ PathScanner.scanForImplementationsArr(FormatPluginConfig.class, packages);
+ if (logger.isDebugEnabled()) {
+ final StringBuilder sb = new StringBuilder();
--- End diff --
Reduce line count by factoring out part that Joiner could be used for.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---