[GitHub] [incubator-ratis] avijayanhwx commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl

[GitBox <gi...@apache.org>]

avijayanhwx commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r418187393



##########
File path: ratis-metrics/src/main/java/org/apache/ratis/metrics/MetricsReporting.java
##########
@@ -34,17 +34,41 @@ private MetricsReporting() {
   }
 
   public static Consumer<RatisMetricRegistry> consoleReporter(TimeDuration rate) {
-    return ratisMetricRegistry -> ConsoleReporter.forRegistry(ratisMetricRegistry.getDropWizardMetricRegistry())
-        .convertRatesTo(TimeUnit.SECONDS).convertDurationsTo(TimeUnit.MILLISECONDS).build()
-        .start(rate.getDuration(), rate.getUnit());
+    return ratisMetricRegistry -> {
+      ConsoleReporter reporter = ConsoleReporter.forRegistry(ratisMetricRegistry.getDropWizardMetricRegistry())
+          .convertRatesTo(TimeUnit.SECONDS).convertDurationsTo(TimeUnit.MILLISECONDS).build();
+      reporter.start(rate.getDuration(), rate.getUnit());
+      ratisMetricRegistry.setConsoleReporter(reporter);
+    };
+  }
+
+  public static Consumer<RatisMetricRegistry> stopConsoleReporter() {

Review comment:
       Do we need to add a stop method for every reporter? Maybe we can rely on the generic close() method implemented by all reports (Closeable) and they already stop() in the close() method?

##########
File path: ratis-metrics/src/main/java/org/apache/ratis/metrics/impl/RatisMetricRegistryImpl.java
##########
@@ -116,4 +123,34 @@ private String getMetricName(String shortName) {
       }
     }
   }
+
+  @Override
+  public void setJmxReporter(JmxReporter jmxReporter) {
+    this.jmxReporter = jmxReporter;
+  }
+
+  @Override
+  public JmxReporter getJmxReporter() {
+    return this.jmxReporter;
+  }
+
+  @Override
+  public void setConsoleReporter(ConsoleReporter consoleReporter) {
+    this.consoleReporter = consoleReporter;
+  }
+
+  @Override
+  public ConsoleReporter getConsoleReporter() {
+    return this.consoleReporter;
+  }
+
+  @Override
+  public void setPrometheusCollector(Collector prometheusCollector) {

Review comment:
       Are we doing this to support a prometheus collector purely in Ratis, instead of depending on an external client like Ozone? 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org