You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ratis.apache.org by GitBox <gi...@apache.org> on 2020/04/30 06:10:54 UTC
[GitHub] [incubator-ratis] runzhiwang opened a new pull request #76: RATIS-845. Memory leak of RaftServerImpl
runzhiwang opened a new pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76
**What's the problem ?**
As the image shows, there are 1885 instances of RaftServerImpl, most of them are Closed, and should be GC, but actually not. You can find from the image
1513 RaftServerImpl were held by ManagermentFactory->jxmMBeanServer->HashMap, 372 RaftServerImpl were held by Datanode ReportManager Thread -> prometheus -> HashMap. So 1513 RaftServerImpl leak in ratis, and 372 leak in ozone. If RaftServerImpl can not GC, there are a lot of related resource can not be GC, such as the [DirectByteBuffer](https://github.com/apache/incubator-ratis/blob/master/ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLogWorker.java#L150) in SegmentRaftLogWorker, which result 1GB memory leak out of heap.
**1. 1885 instances of RaftServerImpl**
**2. 1513 RaftServerImpl were held by ManagermentFactory->jxmMBeanServer->HashMap, 372 RaftServerImpl were held by Datanode ReportManager Thread -> prometheus -> HashMap**
**3. 1513 RaftServerImpl were held by ManagermentFactory->jxmMBeanServer->HashMap**
**4. 372 RaftServerImpl were held by Datanode ReportManager Thread -> prometheus -> HashMap**
5. 2038 DirectByteBuffer, and 1885 held by RaftServerImpl
6. 1033 DirectByteBuffer were held by ManagermentFactory, 802 DirectByteBuffer were held by Datanode ReportManager Thread, total 1885.
7. The reason RaftServerImpl held by ManagermentFactory->jxmMBeanServer->HashMap is ratis start [JmxReporter](https://github.com/apache/incubator-ratis/blob/master/ratis-metrics/src/main/java/org/apache/ratis/metrics/MetricsReporting.java#L47), but does not stop it.
8. The reason RaftServerImpl held by Datanode ReportManager Thread -> prometheus -> HashMap is ozone call the ratis function to [register](https://github.com/apache/hadoop-ozone/blob/master/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java#L189) metric in prometheus, but does not unregister it.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] bshashikant commented on pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
bshashikant commented on pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#issuecomment-621653175
@avijayanhwx, can you plz look at this ?
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] avijayanhwx commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r420393046
##########
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:
@runzhiwang Thanks, but IMHO, given that the JMX and console reporters' implementations are present in Ratis code itself, we can expose an APIs that look like enableJmxReporter(), enableConsoleReporters(), and the current addReporterRegistration API can be used for external reporters like Prometheus. This keeps a client code simple, but provides scope for adding custom reporters. What do you think?
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r418450444
##########
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:
@avijayanhwx Thanks for review. I have remove prometheus in Ratis, I handle this in 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
[GitHub] [incubator-ratis] bshashikant merged pull request #76: RATIS-845. Fix memory leak of RaftServerImpl for no unregister from reporter
Posted by GitBox <gi...@apache.org>.
bshashikant merged pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] avijayanhwx commented on pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#issuecomment-622009122
cc @elek since he added the reporter registrations.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] avijayanhwx commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r420393046
##########
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:
@runzhiwang Thanks, but IMHO, given that the JMX and console reporters' implementation are present in Ratis code itself, we can expose an APIs that look like enableJmxReporter(), enableConsoleReporters(), and the current addReporterRegistration API can be used for external reporters like Prometheus. This keeps a client code simple, but provides scope for adding custom reporters.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] avijayanhwx commented on pull request #76: RATIS-845. Fix memory leak of RaftServerImpl for no unregister from reporter
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#issuecomment-628786400
> Thanks @runzhiwang. I am +1 for this patch.
My vote is non-binding since I am not a committer. cc @bshashikant.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r420489319
##########
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:
@avijayanhwx Thanks for explanation. I agree with you. I have exposed enableJmxReporter(), enableConsoleReporters().
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] avijayanhwx commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r418616987
##########
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:
Thank you @runzhiwang. Can we use the close method now, given that you have removed the prometheus reporter registrations? Is that OK with you?
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #76: [WIP]RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r418216518
##########
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:
@avijayanhwx Yes, we can close() JmxReporter and ConsoleReporter. But for prometheus, it only has unregister. The following code copy from ozone related PR: https://github.com/apache/hadoop-ozone/pull/885.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] avijayanhwx commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl for no unregister from reporter
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r420569786
##########
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:
Thanks @runzhiwang. The changes look good.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r418451235
##########
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:
@avijayanhwx For ConsoleReporter and JmxReporter we can call close. But for prometheus in ozone, there is only unregister() method. The following code copied from https://github.com/apache/hadoop-ozone/pull/885.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl for no unregister from reporter
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r420572735
##########
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);
Review comment:
@avijayanhwx Because when we stop the reporter, we must get it from registry, so we need to set it in registry when create. Such as the following code, we need to getConsoleReporter from registry.
```
return ratisMetricRegistry -> {
ConsoleReporter reporter = ratisMetricRegistry.getConsoleReporter();
if (reporter != null) {
reporter.close();
}
};
```
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r418791782
##########
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:
@avijayanhwx Hi, thanks for review. Even though I have removed prometheus in ratis, but it exists in ozone. Such as the following code. ozone call `CollectorRegistry.defaultRegistry.register` to register registry in prometheus, and it also need call `CollectorRegistry.defaultRegistry.unregister` to unregister it. There is no close method for prometheus, and the unregister also need a parameter which differs from Reporter::close.
```
MetricRegistries.global().addReporterRegistration(
registry -> CollectorRegistry.defaultRegistry.register(
new RatisDropwizardExports(
registry.getDropWizardMetricRegistry())),
registry -> CollectorRegistry.defaultRegistry.unregister(
new RatisDropwizardExports(
registry.getDropWizardMetricRegistry())));
```
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] avijayanhwx commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by 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 call stop() in the close() method?
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r418451235
##########
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:
@avijayanhwx For ConsoleReporter and JmxReporter we can call close. But for prometheus in ozone, there is only unregister() method. The following code copied from https://github.com/apache/hadoop-ozone/pull/885.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] avijayanhwx commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r420393046
##########
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:
@runzhiwang Thanks, but IMHO, given that the JMX and console reporters' implementation are present in Ratis code itself, we can expose an APIs that look like enableJmxReporter(), enableConsoleReporters(), and the current addReporterRegistration API can be used for external reporters like Prometheus. This keeps a client code simple, but provides scope for adding custom reporters. What do you think?
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r418199776
##########
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:
@avijayanhwx Thanks for review. Actually, I also do not want to add prometheus in Ratis.
But because ozone pass a lambda to ratis to register collector in prometheus, and if we want to unregister the collector when remove registry by `CollectorRegistry.defaultRegistry.unregister(collector)`, we must record the collector in Ratis, if you have better idea, plz share it. The following code copy from ozone related PR: https://github.com/apache/hadoop-ozone/pull/885.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #76: [WIP]RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r418216518
##########
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:
@avijayanhwx Yes, we can close() JmxReporter and ConsoleReporter. But for prometheus, it only has unregister. The following code copy from ozone related PR: https://github.com/apache/hadoop-ozone/pull/885.
##########
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:
@avijayanhwx Thanks for review. Actually, I also do not want to add prometheus in Ratis.
But because ozone pass a lambda to ratis to register collector in prometheus, and if we want to unregister the collector when remove registry by `CollectorRegistry.defaultRegistry.unregister(collector)`, we must record the collector in Ratis, if you have better idea, plz share it. The following code copy from ozone related PR: https://github.com/apache/hadoop-ozone/pull/885.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] avijayanhwx commented on pull request #76: RATIS-845. Fix memory leak of RaftServerImpl for no unregister from reporter
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#issuecomment-625027972
Thanks @runzhiwang. I am +1 for this patch.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] avijayanhwx commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl for no unregister from reporter
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r420569786
##########
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:
Thanks @runzhiwang.
##########
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);
Review comment:
Can you tell me why we need this change? Instead of returning a function that starts the console reporter for a given RatisMetricRegistry, why are we starting and using a setter?
`ratisMetricRegistry.setConsoleReporter(reporter);`
##########
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() {
+ return ratisMetricRegistry -> {
+ ConsoleReporter reporter = ratisMetricRegistry.getConsoleReporter();
+ if (reporter != null) {
+ reporter.close();
+ }
+ };
}
public static Consumer<RatisMetricRegistry> jmxReporter() {
return registry -> {
Builder builder =
JmxReporter.forRegistry(registry.getDropWizardMetricRegistry());
builder.inDomain(registry.getMetricRegistryInfo().getApplicationName());
- builder.build().start();
+ JmxReporter reporter = builder.build();
+ reporter.start();
+
+ registry.setJmxReporter(reporter);
Review comment:
Same here.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r418216518
##########
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:
@avijayanhwx Yes, we can close() JmxReporter and ConsoleReporter. But for prometheus, it only has unregister, and also need a parameter i.e.collection. I think prometheus make the problem complicated.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r418199776
##########
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:
@avijayanhwx Thanks for review. Actually, I also do not want to add prometheus in Ratis.
But because ozone pass a lambda to ratis to register collector in prometheus, and if we want to unregister the collector when remove registry by `CollectorRegistry.defaultRegistry.unregister(collector)`, we must record the collector in Ratis, if you have better idea, plz share it. The following code copy from ozone related PR: https://github.com/apache/hadoop-ozone/pull/885.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r418216518
##########
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:
@avijayanhwx Yes, we can close() JmxReporter and ConsoleReporter. But for prometheus, it only has unregister, and also need a parameter i.e.collector. I think prometheus make the problem complicated.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] avijayanhwx commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r420393046
##########
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:
@runzhiwang Thanks, but IMHO, given that the JMX and console reporters' implementations are present in Ratis code itself, we can expose APIs like enableJmxReporter(), enableConsoleReporters(), and the current addReporterRegistration API can be used for external reporters like Prometheus. This keeps a client code simple, but provides scope for adding custom reporters. What do you think?
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] runzhiwang commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #76:
URL: https://github.com/apache/incubator-ratis/pull/76#discussion_r418216518
##########
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:
@avijayanhwx Yes, we can close() JmxReporter and ConsoleReporter. But for prometheus, it only has unregister, and also need a parameter i.e.collector. I think prometheus make the problem complicated.The following code copy from ozone related PR: https://github.com/apache/hadoop-ozone/pull/885.
----------------------------------------------------------------
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
[GitHub] [incubator-ratis] avijayanhwx commented on a change in pull request #76: RATIS-845. Fix memory leak of RaftServerImpl
Posted by 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