You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by srdo <gi...@git.apache.org> on 2018/08/16 16:32:31 UTC
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
GitHub user srdo opened a pull request:
https://github.com/apache/storm/pull/2805
STORM-3197: Make StormMetricsRegistry non-static
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 https://github.com/apache/storm/pull/2783
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/srdo/storm STORM-3197
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2805.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 #2805
----
commit 3044239a841c96472eaedbb645e9fb29a32dcfb4
Author: Stig Rohde Døssing <sr...@...>
Date: 2018-07-30T20:53:08Z
STORM-3197: Make StormMetricsRegistry non-static
----
---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2805#discussion_r218150464
--- Diff: storm-server/src/main/java/org/apache/storm/LocalDRPC.java ---
@@ -38,9 +39,9 @@
private final DRPC drpc;
private final String serviceId;
- public LocalDRPC() {
+ public LocalDRPC(StormMetricsRegistry metricsRegistry) {
--- End diff --
Added a no-args constructor that creates a new registry.
---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2805#discussion_r218148492
--- Diff: storm-server/src/main/java/org/apache/storm/daemon/supervisor/Container.java ---
@@ -219,11 +201,11 @@ public void kill() throws IOException {
shutdownTimer = shutdownDuration.time();
}
try {
- Set<Long> pids = getAllPids();
+ Set<Long> pids = getAllPids();
- for (Long pid : pids) {
- kill(pid);
- }
+ for (Long pid : pids) {
+ kill(pid);
+ }
--- End diff --
Will reformat
---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2805#discussion_r210688568
--- Diff: pom.xml ---
@@ -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>
--- End diff --
Raised https://issues.apache.org/jira/browse/STORM-3199 to get rid of metrics-ganglia
---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2805#discussion_r218107287
--- Diff: storm-server/src/main/java/org/apache/storm/daemon/supervisor/Container.java ---
@@ -219,11 +201,11 @@ public void kill() throws IOException {
shutdownTimer = shutdownDuration.time();
}
try {
- Set<Long> pids = getAllPids();
+ Set<Long> pids = getAllPids();
- for (Long pid : pids) {
- kill(pid);
- }
+ for (Long pid : pids) {
+ kill(pid);
+ }
--- End diff --
It looks like indentation is off around here. could be spaces vs tabs or something, not really sure.
---
[GitHub] storm issue #2805: STORM-3197: Make StormMetricsRegistry non-static
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2805
Also the merge conflicts are really minor.
---
[GitHub] storm issue #2805: STORM-3197: Make StormMetricsRegistry non-static
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2805
@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.
---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on a diff in the pull request:
https://github.com/apache/storm/pull/2805#discussion_r210690260
--- Diff: pom.xml ---
@@ -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>
--- End diff --
Thanks. Just like to have a trail for version changes and the requirements.
---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2805#discussion_r218106022
--- Diff: storm-server/src/main/java/org/apache/storm/LocalDRPC.java ---
@@ -38,9 +39,9 @@
private final DRPC drpc;
private final String serviceId;
- public LocalDRPC() {
+ public LocalDRPC(StormMetricsRegistry metricsRegistry) {
--- End diff --
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.
---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2805#discussion_r210685351
--- Diff: pom.xml ---
@@ -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>
--- End diff --
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. https://github.com/dropwizard/metrics/issues/1319
---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2805
---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on a diff in the pull request:
https://github.com/apache/storm/pull/2805#discussion_r210680050
--- Diff: pom.xml ---
@@ -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>
--- End diff --
can you comment on the version change?
---
[GitHub] storm issue #2805: STORM-3197: Make StormMetricsRegistry non-static
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the issue:
https://github.com/apache/storm/pull/2805
@revans2 Looks like we have few conflicts on this PR?
---
[GitHub] storm issue #2805: STORM-3197: Make StormMetricsRegistry non-static
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2805
Test failure looks unrelated, it's getting a permission error when writing to the local maven repo.
---
[GitHub] storm pull request #2805: STORM-3197: Make StormMetricsRegistry non-static
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2805#discussion_r210684974
--- Diff: pom.xml ---
@@ -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>
--- End diff --
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.
---