You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by mikewalch <gi...@git.apache.org> on 2017/03/28 16:41:46 UTC
[GitHub] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
GitHub user mikewalch opened a pull request:
https://github.com/apache/accumulo/pull/236
ACCUMULO-4614 AccumuloClassLoader should load accumulo-site.xml from \u2026
\u2026classpath
* Removed use of ACCUMULO_CONF_DIR env variable
* Updated accumulo command to no longer require accumulo-site.xml to exist in conf dir
* Made minor documentation & comment changes
* Added debug messages to print the path of the accumulo-site.xml that was loaded or
warn if it was not found on classpath.
* Improved method names in AccumuloClassLoader
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mikewalch/accumulo accumulo-site
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/236.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 #236
----
----
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108553471
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
@mikewalch It sounds like you're suggesting that the only reason to offer both is because a user might have poor configuration management practices, resulting in unexpected configuration changes. For such a user, one could say that the contents of the file at the given absolute path are just as likely to change unexpectedly.
As long as the file URI scheme works correctly, I won't veto it or anything. But, I do think the argument in favor of it is somewhat weak. I also have some concerns that it might set a bad precedent. By supporting both, are we setting ourselves up to ensure that we support both mechanisms for all future configs, (metrics, client configs, etc.), for consistency?
It's unfortunate that log4j doesn't ship its configuration loading strategy as a separate component that we could reuse. If they did, we could just use that for all configuration file loading, and get consistency across the board with minimal code to maintain.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108482950
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
`SiteConfiguration` loads `accumulo-site.xml` from classpath so this is just following what it does.
We could rename the system property `org.apache.accumulo.config.file` to `accumulo.configuration`. It could still default to `accumulo-site.xml` and load from the classpath. However, I think the code could be modified to support setting `accumulo.configuration` to `file://path/to/accumulo-site.xml` to support loading configuraiton from the filesystem without having to add configuration to the classpath. I will look into this.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108681934
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
I think there are other cases where the `file://` feature is useful such as a user cannot modify the classpath easily (ex. unit test) or doesn't want the directory where their accumulo-site.xml is located placed on classpath. These are rare cases but the code for `file://` feature is small and the default behavior of loading from the classpath has not changed.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108550201
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
The classpath can change and accumulo-site.xml could be loaded from an unexpected location. This gives users the ability to be very explicit about where configuration will be loaded.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108535021
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
> it would be nice if somehow users could set an absolute path to a configuration file on the file system.
Any reason to not make it look like a URI and only accept "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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108531362
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
@joshelser Please try to "assume good intentions", rather than expect everybody (or maybe just me?) to preface opinions with "It is my opinion that" or thoughts with "I think that". Assuming good intentions on the receiving end is a lot less work than expecting everybody on the sending end to conform to a particular preferred pattern of speech.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108534597
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
@ctubbsii, I did not know that classpath resources could have a leading slash. I will revert the commit but I still think it would be nice if somehow users could set an absolute path to a configuration file on the file system.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108520921
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
> I think this behaviour is useful to have and doesn't add too much code
+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.
---
[GitHub] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108488065
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
@mikewalch The "file://" scheme (and possibly other registered schemes) are supported by Log4j2 using its NetUtils class. It's non-trivial to do it right, as you can tell by looking at their code. I don't recommend it. We don't need this.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108484735
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
Log4j2 has a *TON* of code to try to support loading from a URI or from the classpath. I'm not sure what Log4j 1.2 does.
In any case, to try to support more could be a nightmare. I don't recommend it. It's pretty easy... and normative for Java apps... to just use the classpath for config resources.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108543513
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
Good idea.. I will push the commit back with that change.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/accumulo/pull/236
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108527201
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
> "We don't need this" isn't a sweeping statement. It's shorthand for "I think we don't need this".
I appreciate your clarification, but this is not how I did (or would) interpret this, you or anyone else.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108526740
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
@joshelser "We don't need this" isn't a sweeping statement. It's shorthand for "I think we don't need this". I just didn't think it necessary to preface all my opinions like that to achieve the desired level of reception.
@mikewalch Your patch won't work. A leading slash is valid for class path resources, and you can't tell what the user intends by branching on that condition. The way log4j does it is with parsing a URI scheme, which is complicated, and bloaty, but is the only way you can add this feature. Again, I don't think the feature is worth adding, though.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108544154
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
> Any reason to not make it look like a URI and only accept "file://"?
There's not much difference between a File URI and other registered URI schemes. Limiting to just file might be slightly easier if we want to just parse the string ourselves in a quick-and-dirty way, rather than use the robust built-in support for URIs. I strongly recommend looking at what log4j2 does first, before implementing any URI scheme support. The ConfigurationFactory, NetUtils, and ConfigurationSource classes are particularly insightful about what it takes to properly parse URI schemes in a robust and consistent way.
I don't understand why the desire to pursue this at all, though. What additional feature would this actually get you? Any file which can be specified with a file URI scheme could be supported by referring to the same file on the classpath instead. What am I missing that makes the URI scheme so appealing?
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108533118
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
> assume good intentions
It has nothing to do with intentions. It has to do with the interpretation of what you wrote. I asked Mike "Could you look at doing this", and your reply concluded with "We don't need this.". I'm pretty sure I would have to call that misinterpreting what you what to treat it how you expected it to.
But, I have absolutely no interest in hashing this out with you. It's a waste time.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108476988
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
Is there a reason to require that accumulo-site.xml be on the classpath? When I saw the PR title, I initially thought about the log4j configuration being able to pull the config file from anywhere.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108493580
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
> override the site.xml file that I want to use
Another option here is to just drop this configuration property and only pick up the configuration file that is (first) found on the classpath.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108517849
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
I pushed another commit that allows `accumulo.configuration` to be set to a path to a configuration file on the file system. If a path is not given, the file will be loaded from the classpath. I think this behaviour is useful to have and doesn't add too much code but I can revert this commit if you guys disagree.
---
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] accumulo pull request #236: ACCUMULO-4614 AccumuloClassLoader should load ac...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/236#discussion_r108492848
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java ---
@@ -44,21 +44,21 @@
public class AccumuloClassLoader {
- public static final String CLASSPATH_PROPERTY_NAME = "general.classpaths";
+ public static final String GENERAL_CLASSPATHS = "general.classpaths";
public static final String MAVEN_PROJECT_BASEDIR_PROPERTY_NAME = "general.maven.project.basedir";
public static final String DEFAULT_MAVEN_PROJECT_BASEDIR_VALUE = "";
- private static String SITE_CONF;
+ private static URL accumuloConfigUrl;
private static URLClassLoader classloader;
private static final Logger log = LoggerFactory.getLogger(AccumuloClassLoader.class);
static {
- String configFile = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
- if (System.getenv("ACCUMULO_CONF_DIR") != null) {
- // accumulo conf dir should be set
- SITE_CONF = System.getenv("ACCUMULO_CONF_DIR") + "/" + configFile;
+ String configFilename = System.getProperty("org.apache.accumulo.config.file", "accumulo-site.xml");
+ accumuloConfigUrl = AccumuloClassLoader.class.getClassLoader().getResource(configFilename);
+ if (accumuloConfigUrl == null) {
+ log.warn("Failed to load Accumulo configuration '{}' from classpath", configFilename);
--- End diff --
> support loading from a URI or from the classpath
Supporting a general URI is way beyond what I originally meant to suggest in loading from any local file.
> We don't need this
Way to go sweeping statements!
I don't much care if the hypothetical value is a URI or not, but I think there *is* value in not tying where configuration files are picked up, especially now that we have much more sane scripts that would let me override the site.xml file that I want to use (e.g. I want to test a config modification without overwriting what is presently there)
---
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.
---