STORM-3197: Make StormMetricsRegistry non-static #2805
Conversation
pom.xml
Outdated
| @@ -280,7 +280,7 @@ | |||
| <log4j-over-slf4j.version>1.6.6</log4j-over-slf4j.version> | |||
| <log4j.version>2.8.2</log4j.version> | |||
| <slf4j.version>1.7.21</slf4j.version> | |||
| <metrics.version>3.1.0</metrics.version> | |||
| <metrics.version>3.2.6</metrics.version> | |||
There was a problem hiding this comment.
can you comment on the version change?
There was a problem hiding this comment.
Yes, sorry. The previous version didn't have the methods from this PR https://github.com/dropwizard/metrics/pull/1023/files. We need them so we can get getOrAdd behavior (either add the metric if not registered, or return the existing metric) for metric registration that uses factory functions. Without this ability, registering metrics (especially gauges) would be a pain, since we'd have to be very careful only to register them once.
There was a problem hiding this comment.
This specific version just happens to be the latest in the 3.x release line. I'd like to upgrade to the latest version of the library, but we'll have to pull out a bit of code relating to the Ganglia reporter first, and I didn't want to mix it in to this PR. dropwizard/metrics#1319
There was a problem hiding this comment.
Raised https://issues.apache.org/jira/browse/STORM-3199 to get rid of metrics-ganglia
There was a problem hiding this comment.
Thanks. Just like to have a trail for version changes and the requirements.
| @@ -38,9 +39,9 @@ | |||
| private final DRPC drpc; | |||
| private final String serviceId; | |||
|
|
|||
| public LocalDRPC() { | |||
| public LocalDRPC(StormMetricsRegistry metricsRegistry) { | |||
There was a problem hiding this comment.
This feels problematic to me.
At a minimum we need to update ./docs/Local-mode.md to have the new way to create it documented, but I would really prefer to just have at least the option for a default constructor. This is local mode the only reason we would need to have a single metrics registry would be if we intended to gather metrics from it afterwards as a part of a test, which we don't really do right now.
There was a problem hiding this comment.
Added a no-args constructor that creates a new registry.
| } | ||
| for (Long pid : pids) { | ||
| kill(pid); | ||
| } |
There was a problem hiding this comment.
It looks like indentation is off around here. could be spaces vs tabs or something, not really sure.
|
Also the merge conflicts are really minor. |
|
@revans2 Looks like we have few conflicts on this PR? |
|
@kishorvpatil yes but the merge conflicts are minor. I wanted to get this reviewed so @srdo can address any review comments at the same time as fixing the merge conflicts so we can get this in ASAP and have a 2.0.0 RC out ASAP. |
|
Test failure looks unrelated, it's getting a permission error when writing to the local maven repo. |
…-3197 STORM-3197: Make StormMetricsRegistry non-static This closes #2805
https://issues.apache.org/jira/browse/STORM-3197
This also fixes https://issues.apache.org/jira/browse/STORM-3101 and https://issues.apache.org/jira/browse/STORM-3173.
We talked about doing this in #2783