xichen01 commented on PR #744:
URL: https://github.com/apache/ratis/pull/744#issuecomment-1248012891
> @xichen01 , thanks a lot for reporting the bug and fixing it! Since the
lists won't change frequently, how about add `synchronized` to all the methods
and use `ArrayList` instead of `CopyOnWriteArrayList`?
>
> ```java
> diff --git
a/ratis-metrics/src/main/java/org/apache/ratis/metrics/impl/MetricRegistriesImpl.java
b/ratis-metrics/src/main/java/org/apache/ratis/metrics/impl/MetricRegistriesImpl.java
> index cbe8d7bf..a459509d 100644
> ---
a/ratis-metrics/src/main/java/org/apache/ratis/metrics/impl/MetricRegistriesImpl.java
> +++
b/ratis-metrics/src/main/java/org/apache/ratis/metrics/impl/MetricRegistriesImpl.java
> @@ -17,12 +17,12 @@
> */
> package org.apache.ratis.metrics.impl;
>
> +import java.util.ArrayList;
> import java.util.Collection;
> import java.util.Collections;
> import java.util.List;
> import java.util.Optional;
> import java.util.Set;
> -import java.util.concurrent.CopyOnWriteArrayList;
> import java.util.function.Consumer;
>
> import org.apache.ratis.metrics.MetricRegistries;
> @@ -41,10 +41,36 @@ public class MetricRegistriesImpl extends
MetricRegistries {
>
> private static final Logger LOG =
LoggerFactory.getLogger(MetricRegistriesImpl.class);
>
> - private final List<Consumer<RatisMetricRegistry>> reporterRegistrations
= new CopyOnWriteArrayList<>();
> + class Reporters {
> + private final List<Consumer<RatisMetricRegistry>> registrations = new
ArrayList<>();
> + private final List<Consumer<RatisMetricRegistry>> stoppers = new
ArrayList<>();
>
> - private final List<Consumer<RatisMetricRegistry>> stopReporters = new
CopyOnWriteArrayList<>();
> + synchronized RatisMetricRegistry create(MetricRegistryInfo info) {
> + System.out.println("isEmpty? " + registrations.isEmpty());
> + if (registrations.isEmpty()) {
> + if (LOG.isDebugEnabled()) {
> + LOG.debug("First MetricRegistry has been created without
registering reporters. "
> + + "Hence registering JMX reporter by default.");
> + }
> + enableJmxReporter();
> + }
> +
> + final RatisMetricRegistry registry = factory.create(info);
> + registrations.forEach(reg -> reg.accept(registry));
> + return registry;
> + }
>
> + synchronized void add(Consumer<RatisMetricRegistry> registration,
Consumer<RatisMetricRegistry> stopper) {
> + registrations.add(registration);
> + stoppers.add(stopper);
> + }
> +
> + synchronized void remove(RatisMetricRegistry registry) {
> + stoppers.forEach(reg -> reg.accept(registry));
> + }
> + }
> +
> + private final Reporters reporters = new Reporters();
> private final MetricRegistryFactory factory;
>
> private final RefCountingMap<MetricRegistryInfo, RatisMetricRegistry>
registries;
> @@ -60,27 +86,12 @@ public class MetricRegistriesImpl extends
MetricRegistries {
>
> @Override
> public RatisMetricRegistry create(MetricRegistryInfo info) {
> - return registries.put(info, () -> {
> - if (reporterRegistrations.isEmpty()) {
> - if (LOG.isDebugEnabled()) {
> - LOG.debug("First MetricRegistry has been created without
registering reporters. " +
> - "Hence registering JMX reporter by default.");
> - }
> - enableJmxReporter();
> - }
> - RatisMetricRegistry registry = factory.create(info);
> - reporterRegistrations.forEach(reg -> reg.accept(registry));
> - return registry;
> - });
> + return registries.put(info, () -> reporters.create(info));
> }
>
> @Override
> public boolean remove(MetricRegistryInfo key) {
> - RatisMetricRegistry registry = registries.get(key);
> - if (registry != null) {
> - stopReporters.forEach(reg -> reg.accept(registry));
> - }
> -
> + get(key).ifPresent(reporters::remove);
> return registries.remove(key) == null;
> }
>
> @@ -111,12 +122,12 @@ public class MetricRegistriesImpl extends
MetricRegistries {
> LOG.warn("New reporters are added after registries were created.
Some metrics will be missing from the reporter. "
> + "Please add reporter before adding any new registry.");
> }
> - this.reporterRegistrations.add(reporterRegistration);
> - this.stopReporters.add(stopReporter);
> + reporters.add(reporterRegistration, stopReporter);
> }
>
> @Override
> public void enableJmxReporter() {
> + System.out.println("enableJmxReporter");
> addReporterRegistration(
> MetricsReporting.jmxReporter(),
> MetricsReporting.stopJmxReporter());
> ```
`ArrayList With synchronized` may be better maintained than
`CopyOnWriteArrayList`, I well try 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
