Github user zd-project commented on a diff in the pull request: https://github.com/apache/storm/pull/2771#discussion_r207636496 --- Diff: storm-server/src/main/java/org/apache/storm/metric/StormMetricsRegistry.java --- @@ -50,19 +61,19 @@ public static void startMetricsReporters(Map<String, Object> topoConf) { } } - private static <T extends Metric> T register(final String name, T metric) { - T ret; + @Override + //This is more similar to super#getOrAdd than super#register + public <T extends Metric> T register(final String name, T metric) throws IllegalArgumentException { --- End diff -- I added a test for registerMetricSet and unregisterMetricSet in #2754 and #2764 to show that this method has solved the issue of double registration. See https://github.com/apache/storm/pull/2754/commits/597c6bb2d41a7aa0d25c6aab9201b451f6b1eaf1 I don't know if we should move the change to `registerMetricSet` and `unregisterMetricSet` back to this PR, but the lack of them seems to be confusing people of the purpose of this change. @Ethanlm @srdo @revans2
---