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/01 20:36:16 UTC
[GitHub] accumulo pull request #225: ACCUMULO-4593 Move metrics config to templates d...
GitHub user mikewalch opened a pull request:
https://github.com/apache/accumulo/pull/225
ACCUMULO-4593 Move metrics config to templates dir
* Updated documentation to reflect change to templates dir
* Legacy metrics now look for configuration file on classpath rather
than using ACCUMULO_CONF_DIR env variable
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mikewalch/accumulo metrics-config
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/225.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 #225
----
commit e22ee4558a096cb90258462cf700c777ae8e8c9b
Author: Mike Walch <mw...@apache.org>
Date: 2017-02-28T21:18:24Z
ACCUMULO-4593 Move metrics config to templates dir
* Updated documentation to reflect change to templates dir
* Legacy metrics now look for configuration file on classpath rather
than using ACCUMULO_CONF_DIR env variable
----
---
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 #225: ACCUMULO-4593 Move metrics config to templates d...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/225#discussion_r103785551
--- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsConfiguration.java ---
@@ -148,46 +148,35 @@ public Configuration getMetricsConfiguration() {
}
private void loadConfiguration() {
- String accumuloConfDir = getEnvironmentConfiguration().getString("ACCUMULO_CONF_DIR");
- if (null != accumuloConfDir) {
- // Try to load the metrics properties file
- File mFile = new File(accumuloConfDir, metricsFileName);
- if (mFile.exists()) {
- if (log.isDebugEnabled())
- log.debug("Loading config file: " + mFile.getAbsolutePath());
- try {
- xConfig = new XMLConfiguration(mFile);
- xConfig.append(getEnvironmentConfiguration());
- xConfig.addConfigurationListener(new MetricsConfigListener());
- xConfig.setReloadingStrategy(new FileChangedReloadingStrategy());
-
- // Start a background Thread that checks a property from the XMLConfiguration
- // every so often to force the FileChangedReloadingStrategy to fire.
- if (null == watcher || !watcher.isAlive()) {
- watcher = new MetricsConfigWatcher();
- watcher.start();
- }
- notFound = false;
- alreadyWarned = false;
- } catch (ConfigurationException ce) {
- log.error("Error reading accumulo-metrics.xml file.");
- notFound = true;
- return;
- }
- } else {
- if (!alreadyWarned)
- log.warn("Unable to find metrics file: " + mFile.getAbsolutePath());
- alreadyWarned = true;
- notFound = true;
- return;
- }
- } else {
+ URL metricsUrl = MetricsConfiguration.class.getClassLoader().getResource("accumulo-metrics.xml");
--- End diff --
So many +1s to this simplification.
---
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 #225: ACCUMULO-4593 Move metrics config to templates d...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/225#discussion_r103942407
--- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsConfiguration.java ---
@@ -148,46 +148,35 @@ public Configuration getMetricsConfiguration() {
}
private void loadConfiguration() {
- String accumuloConfDir = getEnvironmentConfiguration().getString("ACCUMULO_CONF_DIR");
- if (null != accumuloConfDir) {
- // Try to load the metrics properties file
- File mFile = new File(accumuloConfDir, metricsFileName);
- if (mFile.exists()) {
- if (log.isDebugEnabled())
- log.debug("Loading config file: " + mFile.getAbsolutePath());
- try {
- xConfig = new XMLConfiguration(mFile);
- xConfig.append(getEnvironmentConfiguration());
- xConfig.addConfigurationListener(new MetricsConfigListener());
- xConfig.setReloadingStrategy(new FileChangedReloadingStrategy());
-
- // Start a background Thread that checks a property from the XMLConfiguration
- // every so often to force the FileChangedReloadingStrategy to fire.
- if (null == watcher || !watcher.isAlive()) {
- watcher = new MetricsConfigWatcher();
- watcher.start();
- }
- notFound = false;
- alreadyWarned = false;
- } catch (ConfigurationException ce) {
- log.error("Error reading accumulo-metrics.xml file.");
- notFound = true;
- return;
- }
- } else {
- if (!alreadyWarned)
- log.warn("Unable to find metrics file: " + mFile.getAbsolutePath());
- alreadyWarned = true;
- notFound = true;
- return;
- }
- } else {
+ URL metricsUrl = MetricsConfiguration.class.getClassLoader().getResource("accumulo-metrics.xml");
+ if (metricsUrl == null) {
if (!alreadyWarned)
- log.warn("ACCUMULO_CONF_DIR variable not found in environment. Metrics collection will be disabled.");
+ log.warn("accumulo-metrics.xml was not found on classpath. Metrics collection will be disabled.");
alreadyWarned = true;
--- End diff --
I am not an expert on this code but I think alreadyWarned is there because the loadConfiguration() can be called multiple times.
---
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 #225: ACCUMULO-4593 Move metrics config to templates d...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/225#discussion_r103797048
--- Diff: docs/src/main/asciidoc/chapters/administration.txt ---
@@ -571,17 +571,20 @@ which allows for routing of metrics generated by registered MetricsSources to co
that are implemented by Hadoop include file-based logging, Graphite and Ganglia. All metric sources are exposed via JMX
when using Metrics2.
-Previous to Accumulo 1.7.0, JMX endpoints could be exposed in addition to file-based logging of those metrics configured via
-the +accumulo-metrics.xml+ file. This mechanism can still be used by setting +general.legacy.metrics+ to +true+ in +accumulo-site.xml+.
+Previous to Accumulo 1.7.0, metrics could be exposed using JMX endpoints or file-based logging. These metrics can be enabled
--- End diff --
See da125e6c80d
---
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 #225: ACCUMULO-4593 Move metrics config to templates d...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/225#discussion_r103914280
--- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsConfiguration.java ---
@@ -148,46 +148,35 @@ public Configuration getMetricsConfiguration() {
}
private void loadConfiguration() {
- String accumuloConfDir = getEnvironmentConfiguration().getString("ACCUMULO_CONF_DIR");
- if (null != accumuloConfDir) {
- // Try to load the metrics properties file
- File mFile = new File(accumuloConfDir, metricsFileName);
- if (mFile.exists()) {
- if (log.isDebugEnabled())
- log.debug("Loading config file: " + mFile.getAbsolutePath());
- try {
- xConfig = new XMLConfiguration(mFile);
- xConfig.append(getEnvironmentConfiguration());
- xConfig.addConfigurationListener(new MetricsConfigListener());
- xConfig.setReloadingStrategy(new FileChangedReloadingStrategy());
-
- // Start a background Thread that checks a property from the XMLConfiguration
- // every so often to force the FileChangedReloadingStrategy to fire.
- if (null == watcher || !watcher.isAlive()) {
- watcher = new MetricsConfigWatcher();
- watcher.start();
- }
- notFound = false;
- alreadyWarned = false;
- } catch (ConfigurationException ce) {
- log.error("Error reading accumulo-metrics.xml file.");
- notFound = true;
- return;
- }
- } else {
- if (!alreadyWarned)
- log.warn("Unable to find metrics file: " + mFile.getAbsolutePath());
- alreadyWarned = true;
- notFound = true;
- return;
- }
- } else {
+ URL metricsUrl = MetricsConfiguration.class.getClassLoader().getResource("accumulo-metrics.xml");
+ if (metricsUrl == null) {
if (!alreadyWarned)
- log.warn("ACCUMULO_CONF_DIR variable not found in environment. Metrics collection will be disabled.");
+ log.warn("accumulo-metrics.xml was not found on classpath. Metrics collection will be disabled.");
alreadyWarned = true;
--- End diff --
With the above simplification, looks like you don't need the `alreadyWarned` boolean anymore.
---
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 #225: ACCUMULO-4593 Move metrics config to templates d...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/225#discussion_r103784837
--- Diff: assemble/bin/accumulo-util ---
@@ -298,7 +298,7 @@ function create_config() {
MAVEN_PROJ_BASEDIR="\n <property>\n <name>general.maven.project.basedir</name>\n <value>${BASE_DIR}</value>\n </property>\n"
fi
- mkdir -p "${CONF_DIR}" && cp "${TEMPLATE_CONF_DIR}"/* "${CONF_DIR}"/
+ mkdir -p "${CONF_DIR}" && cp "${TEMPLATE_CONF_DIR}"/{$ACCUMULO_SITE,$ACCUMULO_ENV} "${CONF_DIR}"/
--- End diff --
You sure this works? You can't use bash variables inside the curly braces for ranges; I'm you could use them for enumerations.
---
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 #225: ACCUMULO-4593 Move metrics config to templates d...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/225#discussion_r103786317
--- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsConfiguration.java ---
@@ -148,46 +148,35 @@ public Configuration getMetricsConfiguration() {
}
private void loadConfiguration() {
- String accumuloConfDir = getEnvironmentConfiguration().getString("ACCUMULO_CONF_DIR");
- if (null != accumuloConfDir) {
- // Try to load the metrics properties file
- File mFile = new File(accumuloConfDir, metricsFileName);
- if (mFile.exists()) {
- if (log.isDebugEnabled())
- log.debug("Loading config file: " + mFile.getAbsolutePath());
- try {
- xConfig = new XMLConfiguration(mFile);
- xConfig.append(getEnvironmentConfiguration());
- xConfig.addConfigurationListener(new MetricsConfigListener());
- xConfig.setReloadingStrategy(new FileChangedReloadingStrategy());
-
- // Start a background Thread that checks a property from the XMLConfiguration
- // every so often to force the FileChangedReloadingStrategy to fire.
- if (null == watcher || !watcher.isAlive()) {
- watcher = new MetricsConfigWatcher();
- watcher.start();
- }
- notFound = false;
- alreadyWarned = false;
- } catch (ConfigurationException ce) {
- log.error("Error reading accumulo-metrics.xml file.");
- notFound = true;
- return;
- }
- } else {
- if (!alreadyWarned)
- log.warn("Unable to find metrics file: " + mFile.getAbsolutePath());
- alreadyWarned = true;
- notFound = true;
- return;
- }
- } else {
+ URL metricsUrl = MetricsConfiguration.class.getClassLoader().getResource("accumulo-metrics.xml");
+ if (metricsUrl == null) {
if (!alreadyWarned)
- log.warn("ACCUMULO_CONF_DIR variable not found in environment. Metrics collection will be disabled.");
+ log.warn("accumulo-metrics.xml was not found on classpath. Metrics collection will be disabled.");
alreadyWarned = true;
notFound = true;
return;
}
+
+ try {
+ xConfig = new XMLConfiguration(metricsUrl);
--- End diff --
This is commons-configuration. Do we really have to force users to use XML for this, when commons-configuration supports the much simpler properties 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] accumulo pull request #225: ACCUMULO-4593 Move metrics config to templates d...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/accumulo/pull/225
---
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 #225: ACCUMULO-4593 Move metrics config to templates d...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/225#discussion_r103785318
--- Diff: docs/src/main/asciidoc/chapters/administration.txt ---
@@ -571,17 +571,20 @@ which allows for routing of metrics generated by registered MetricsSources to co
that are implemented by Hadoop include file-based logging, Graphite and Ganglia. All metric sources are exposed via JMX
when using Metrics2.
-Previous to Accumulo 1.7.0, JMX endpoints could be exposed in addition to file-based logging of those metrics configured via
-the +accumulo-metrics.xml+ file. This mechanism can still be used by setting +general.legacy.metrics+ to +true+ in +accumulo-site.xml+.
+Previous to Accumulo 1.7.0, metrics could be exposed using JMX endpoints or file-based logging. These metrics can be enabled
--- End diff --
s/Previous/Prior/
This section talks about what you could do before, which implies it's different now. But this dangling implication is never connected to anything which follows. Should we say something like "Since 1.7.0, we also support .... for metrics." to connect the ideas?
---
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 #225: ACCUMULO-4593 Move metrics config to templates d...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/225#discussion_r103954159
--- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsConfiguration.java ---
@@ -148,46 +148,35 @@ public Configuration getMetricsConfiguration() {
}
private void loadConfiguration() {
- String accumuloConfDir = getEnvironmentConfiguration().getString("ACCUMULO_CONF_DIR");
- if (null != accumuloConfDir) {
- // Try to load the metrics properties file
- File mFile = new File(accumuloConfDir, metricsFileName);
- if (mFile.exists()) {
- if (log.isDebugEnabled())
- log.debug("Loading config file: " + mFile.getAbsolutePath());
- try {
- xConfig = new XMLConfiguration(mFile);
- xConfig.append(getEnvironmentConfiguration());
- xConfig.addConfigurationListener(new MetricsConfigListener());
- xConfig.setReloadingStrategy(new FileChangedReloadingStrategy());
-
- // Start a background Thread that checks a property from the XMLConfiguration
- // every so often to force the FileChangedReloadingStrategy to fire.
- if (null == watcher || !watcher.isAlive()) {
- watcher = new MetricsConfigWatcher();
- watcher.start();
- }
- notFound = false;
- alreadyWarned = false;
- } catch (ConfigurationException ce) {
- log.error("Error reading accumulo-metrics.xml file.");
- notFound = true;
- return;
- }
- } else {
- if (!alreadyWarned)
- log.warn("Unable to find metrics file: " + mFile.getAbsolutePath());
- alreadyWarned = true;
- notFound = true;
- return;
- }
- } else {
+ URL metricsUrl = MetricsConfiguration.class.getClassLoader().getResource("accumulo-metrics.xml");
+ if (metricsUrl == null) {
if (!alreadyWarned)
- log.warn("ACCUMULO_CONF_DIR variable not found in environment. Metrics collection will be disabled.");
+ log.warn("accumulo-metrics.xml was not found on classpath. Metrics collection will be disabled.");
alreadyWarned = true;
--- End diff --
I think the checks in getMetricsConfiguration() take care of that.
---
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 #225: ACCUMULO-4593 Move metrics config to templates d...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/225#discussion_r103789283
--- Diff: assemble/bin/accumulo-util ---
@@ -298,7 +298,7 @@ function create_config() {
MAVEN_PROJ_BASEDIR="\n <property>\n <name>general.maven.project.basedir</name>\n <value>${BASE_DIR}</value>\n </property>\n"
fi
- mkdir -p "${CONF_DIR}" && cp "${TEMPLATE_CONF_DIR}"/* "${CONF_DIR}"/
+ mkdir -p "${CONF_DIR}" && cp "${TEMPLATE_CONF_DIR}"/{$ACCUMULO_SITE,$ACCUMULO_ENV} "${CONF_DIR}"/
--- End diff --
Yes. I tested it and it works. Shell check doesn't complain either.
---
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 #225: ACCUMULO-4593 Move metrics config to templates d...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/225#discussion_r103791991
--- Diff: server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsConfiguration.java ---
@@ -148,46 +148,35 @@ public Configuration getMetricsConfiguration() {
}
private void loadConfiguration() {
- String accumuloConfDir = getEnvironmentConfiguration().getString("ACCUMULO_CONF_DIR");
- if (null != accumuloConfDir) {
- // Try to load the metrics properties file
- File mFile = new File(accumuloConfDir, metricsFileName);
- if (mFile.exists()) {
- if (log.isDebugEnabled())
- log.debug("Loading config file: " + mFile.getAbsolutePath());
- try {
- xConfig = new XMLConfiguration(mFile);
- xConfig.append(getEnvironmentConfiguration());
- xConfig.addConfigurationListener(new MetricsConfigListener());
- xConfig.setReloadingStrategy(new FileChangedReloadingStrategy());
-
- // Start a background Thread that checks a property from the XMLConfiguration
- // every so often to force the FileChangedReloadingStrategy to fire.
- if (null == watcher || !watcher.isAlive()) {
- watcher = new MetricsConfigWatcher();
- watcher.start();
- }
- notFound = false;
- alreadyWarned = false;
- } catch (ConfigurationException ce) {
- log.error("Error reading accumulo-metrics.xml file.");
- notFound = true;
- return;
- }
- } else {
- if (!alreadyWarned)
- log.warn("Unable to find metrics file: " + mFile.getAbsolutePath());
- alreadyWarned = true;
- notFound = true;
- return;
- }
- } else {
+ URL metricsUrl = MetricsConfiguration.class.getClassLoader().getResource("accumulo-metrics.xml");
+ if (metricsUrl == null) {
if (!alreadyWarned)
- log.warn("ACCUMULO_CONF_DIR variable not found in environment. Metrics collection will be disabled.");
+ log.warn("accumulo-metrics.xml was not found on classpath. Metrics collection will be disabled.");
alreadyWarned = true;
notFound = true;
return;
}
+
+ try {
+ xConfig = new XMLConfiguration(metricsUrl);
--- End diff --
XMLConfiguration is historical. I am only changing how metrics configuration file is found.
---
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 #225: ACCUMULO-4593 Move metrics config to templates d...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/225#discussion_r103792135
--- Diff: docs/src/main/asciidoc/chapters/administration.txt ---
@@ -571,17 +571,20 @@ which allows for routing of metrics generated by registered MetricsSources to co
that are implemented by Hadoop include file-based logging, Graphite and Ganglia. All metric sources are exposed via JMX
when using Metrics2.
-Previous to Accumulo 1.7.0, JMX endpoints could be exposed in addition to file-based logging of those metrics configured via
-the +accumulo-metrics.xml+ file. This mechanism can still be used by setting +general.legacy.metrics+ to +true+ in +accumulo-site.xml+.
+Previous to Accumulo 1.7.0, metrics could be exposed using JMX endpoints or file-based logging. These metrics can be enabled
--- End diff --
I will 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.
---