Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2789#discussion_r208005061
--- Diff:
storm-server/src/main/java/org/apache/storm/daemon/metrics/reporters/PreparableReporter.java
---
@@ -13,16 +13,35 @@
package org.apache.storm.daemon.metrics.reporters;
import com.codahale.metrics.MetricRegistry;
-import com.codahale.metrics.Reporter;
-import java.io.Closeable;
import java.util.Map;
+import java.util.concurrent.TimeUnit;
+import com.codahale.metrics.ScheduledReporter;
+import org.slf4j.Logger;
-public interface PreparableReporter<T extends Reporter & Closeable> {
+public interface PreparableReporter {
void prepare(MetricRegistry metricsRegistry, Map<String, Object>
topoConf);
void start();
void stop();
+ static <T, U extends ScheduledReporter> void
startScheduledReporter(Class<T> enclosingClazz, U reporter, final Logger log) {
--- End diff --
I don't think there's a good reason to have these static methods. If you
want to deduplicate the methods in the implementations, it would probably be
better to do as a collaborator object. If you make a new class that contains
the functionality from these two methods, you can avoid exposing these methods
on the interface, and likely get a nicer method signature as well.
What I mean is something like
```
class ReporterStarter<T extends Reporter> {
private final T reporter;
public void startReporter() {
the implementation of startScheduledReporter goes here
}
}
```
and then you just make the PreparableReporter instances use instances of
that class.
On the other hand, I'd also be okay with not worrying about deduplicating
the methods, it's a very slight amount of code duplication, and I'm not sure
the extra abstraction helps readability.
---
