From 5d9b9d7a23447c810d53a85c63eab122099b2d2f Mon Sep 17 00:00:00 2001 From: Andrzej Bialecki Date: Mon, 21 Aug 2017 15:24:29 +0200 Subject: [PATCH] SOLR-11255: Use concurrent set for metric names. --- solr/CHANGES.txt | 2 + .../java/org/apache/solr/core/SolrCore.java | 2 +- .../org/apache/solr/core/SolrInfoBean.java | 1 + .../solr/handler/RequestHandlerBase.java | 4 +- .../handler/component/SearchComponent.java | 4 +- .../highlight/HighlightingPluginBase.java | 4 +- .../solr/search/SolrFieldCacheBean.java | 4 +- .../apache/solr/search/SolrIndexSearcher.java | 4 +- .../apache/solr/store/blockcache/Metrics.java | 5 +- .../solr/store/hdfs/HdfsLocalityReporter.java | 3 +- .../org/apache/solr/update/UpdateHandler.java | 4 +- .../solr/update/UpdateShardHandler.java | 4 +- .../org/apache/solr/core/MockInfoBean.java | 4 +- .../solr/handler/admin/MBeansHandlerTest.java | 76 +++++++++++++++++++ 14 files changed, 99 insertions(+), 22 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index f0307b076b1..69be893f346 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -97,6 +97,8 @@ Bug Fixes * SOLR-11084: Issue with starting script with solr.home (-s) == solr (Leil Ireson, Amrit Sarkar via Erick Erickson) +* SOLR-11255: Fix occasional ConcurrentModificationException when using SolrInfoMBeanHandler. (ab) + Optimizations ---------------------- diff --git a/solr/core/src/java/org/apache/solr/core/SolrCore.java b/solr/core/src/java/org/apache/solr/core/SolrCore.java index 53708f18965..393bfe6ff79 100644 --- a/solr/core/src/java/org/apache/solr/core/SolrCore.java +++ b/solr/core/src/java/org/apache/solr/core/SolrCore.java @@ -222,7 +222,7 @@ public final class SolrCore implements SolrInfoBean, SolrMetricProducer, Closeab private Counter newSearcherOtherErrorsCounter; private final CoreContainer coreContainer; - private Set metricNames = new HashSet<>(); + private Set metricNames = ConcurrentHashMap.newKeySet(); public Set getMetricNames() { return metricNames; diff --git a/solr/core/src/java/org/apache/solr/core/SolrInfoBean.java b/solr/core/src/java/org/apache/solr/core/SolrInfoBean.java index 472b15e0819..edded0e46a6 100644 --- a/solr/core/src/java/org/apache/solr/core/SolrInfoBean.java +++ b/solr/core/src/java/org/apache/solr/core/SolrInfoBean.java @@ -66,6 +66,7 @@ public interface SolrInfoBean { * Modifiable set of metric names that this component reports (default is null, * which means none). If not null then this set is used by {@link #registerMetricName(String)} * to capture what metrics names are reported from this component. + *

NOTE: this set has to allow iteration under modifications.

*/ default Set getMetricNames() { return null; diff --git a/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java b/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java index 990d475adc3..db2b2ac13cc 100644 --- a/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java +++ b/solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java @@ -18,8 +18,8 @@ package org.apache.solr.handler; import java.lang.invoke.MethodHandles; import java.util.Collection; -import java.util.HashSet; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import com.codahale.metrics.MetricRegistry; import com.google.common.collect.ImmutableList; @@ -74,7 +74,7 @@ public abstract class RequestHandlerBase implements SolrRequestHandler, SolrInfo private PluginInfo pluginInfo; - private Set metricNames = new HashSet<>(); + private Set metricNames = ConcurrentHashMap.newKeySet(); private MetricRegistry registry; @SuppressForbidden(reason = "Need currentTimeMillis, used only for stats output") diff --git a/solr/core/src/java/org/apache/solr/handler/component/SearchComponent.java b/solr/core/src/java/org/apache/solr/handler/component/SearchComponent.java index c615c5a7ac1..78a62fada26 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/SearchComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/SearchComponent.java @@ -19,9 +19,9 @@ package org.apache.solr.handler.component; import java.io.IOException; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import com.codahale.metrics.MetricRegistry; import org.apache.solr.common.util.NamedList; @@ -42,7 +42,7 @@ public abstract class SearchComponent implements SolrInfoBean, NamedListInitiali */ private String name = this.getClass().getName(); - protected Set metricNames = new HashSet<>(); + protected Set metricNames = ConcurrentHashMap.newKeySet(); protected MetricRegistry registry; /** diff --git a/solr/core/src/java/org/apache/solr/highlight/HighlightingPluginBase.java b/solr/core/src/java/org/apache/solr/highlight/HighlightingPluginBase.java index 7acaacdd03c..2620ec7eaab 100644 --- a/solr/core/src/java/org/apache/solr/highlight/HighlightingPluginBase.java +++ b/solr/core/src/java/org/apache/solr/highlight/HighlightingPluginBase.java @@ -16,8 +16,8 @@ */ package org.apache.solr.highlight; -import java.util.HashSet; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import com.codahale.metrics.Counter; import com.codahale.metrics.MetricRegistry; @@ -35,7 +35,7 @@ public abstract class HighlightingPluginBase implements SolrInfoBean, SolrMetric { protected Counter numRequests; protected SolrParams defaults; - protected Set metricNames = new HashSet<>(1); + protected Set metricNames = ConcurrentHashMap.newKeySet(1); protected MetricRegistry registry; public void init(NamedList args) { diff --git a/solr/core/src/java/org/apache/solr/search/SolrFieldCacheBean.java b/solr/core/src/java/org/apache/solr/search/SolrFieldCacheBean.java index ffcc37d64cf..82787d16180 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrFieldCacheBean.java +++ b/solr/core/src/java/org/apache/solr/search/SolrFieldCacheBean.java @@ -16,8 +16,8 @@ */ package org.apache.solr.search; -import java.util.HashSet; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import com.codahale.metrics.MetricRegistry; import org.apache.solr.core.SolrInfoBean; @@ -36,7 +36,7 @@ public class SolrFieldCacheBean implements SolrInfoBean, SolrMetricProducer { private boolean disableJmxEntryList = Boolean.getBoolean("disableSolrFieldCacheMBeanEntryListJmx"); private MetricRegistry registry; - private Set metricNames = new HashSet<>(); + private Set metricNames = ConcurrentHashMap.newKeySet(); @Override public String getName() { return this.getClass().getName(); } diff --git a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java index 599fd2461bc..6a178cb1a91 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java +++ b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java @@ -25,11 +25,11 @@ import java.util.Collections; import java.util.Comparator; import java.util.Date; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; @@ -137,7 +137,7 @@ public class SolrIndexSearcher extends IndexSearcher implements Closeable, SolrI private final String path; private boolean releaseDirectory; - private Set metricNames = new HashSet<>(); + private Set metricNames = ConcurrentHashMap.newKeySet(); private static DirectoryReader getReader(SolrCore core, SolrIndexConfig config, DirectoryFactory directoryFactory, String path) throws IOException { diff --git a/solr/core/src/java/org/apache/solr/store/blockcache/Metrics.java b/solr/core/src/java/org/apache/solr/store/blockcache/Metrics.java index b8b9bea11ab..cc7ae4b5013 100644 --- a/solr/core/src/java/org/apache/solr/store/blockcache/Metrics.java +++ b/solr/core/src/java/org/apache/solr/store/blockcache/Metrics.java @@ -16,8 +16,8 @@ */ package org.apache.solr.store.blockcache; -import java.util.HashSet; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicLong; import com.codahale.metrics.MetricRegistry; @@ -55,8 +55,7 @@ public class Metrics extends SolrCacheBase implements SolrInfoBean, SolrMetricPr private MetricsMap metricsMap; private MetricRegistry registry; - private Set metricNames = new HashSet<>(); - + private Set metricNames = ConcurrentHashMap.newKeySet(); private long previous = System.nanoTime(); @Override diff --git a/solr/core/src/java/org/apache/solr/store/hdfs/HdfsLocalityReporter.java b/solr/core/src/java/org/apache/solr/store/hdfs/HdfsLocalityReporter.java index 64e6356dcd8..3474d3c703f 100644 --- a/solr/core/src/java/org/apache/solr/store/hdfs/HdfsLocalityReporter.java +++ b/solr/core/src/java/org/apache/solr/store/hdfs/HdfsLocalityReporter.java @@ -19,7 +19,6 @@ package org.apache.solr.store.hdfs; import java.io.IOException; import java.lang.invoke.MethodHandles; import java.util.Arrays; -import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -51,7 +50,7 @@ public class HdfsLocalityReporter implements SolrInfoBean, SolrMetricProducer { private String hostname; private final ConcurrentMap> cache; - private final Set metricNames = new HashSet<>(); + private final Set metricNames = ConcurrentHashMap.newKeySet(); private MetricRegistry registry; public HdfsLocalityReporter() { diff --git a/solr/core/src/java/org/apache/solr/update/UpdateHandler.java b/solr/core/src/java/org/apache/solr/update/UpdateHandler.java index f0eb8bc7e12..ff94bfc547e 100644 --- a/solr/core/src/java/org/apache/solr/update/UpdateHandler.java +++ b/solr/core/src/java/org/apache/solr/update/UpdateHandler.java @@ -19,9 +19,9 @@ package org.apache.solr.update; import java.io.IOException; import java.lang.invoke.MethodHandles; -import java.util.HashSet; import java.util.Set; import java.util.Vector; +import java.util.concurrent.ConcurrentHashMap; import com.codahale.metrics.MetricRegistry; import org.apache.solr.core.DirectoryFactory; @@ -58,7 +58,7 @@ public abstract class UpdateHandler implements SolrInfoBean { protected final UpdateLog ulog; - protected Set metricNames = new HashSet<>(); + protected Set metricNames = ConcurrentHashMap.newKeySet(); protected MetricRegistry registry; private void parseEventListeners() { diff --git a/solr/core/src/java/org/apache/solr/update/UpdateShardHandler.java b/solr/core/src/java/org/apache/solr/update/UpdateShardHandler.java index 20132e133d7..4ad51060889 100644 --- a/solr/core/src/java/org/apache/solr/update/UpdateShardHandler.java +++ b/solr/core/src/java/org/apache/solr/update/UpdateShardHandler.java @@ -17,8 +17,8 @@ package org.apache.solr.update; import java.lang.invoke.MethodHandles; -import java.util.HashSet; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; import java.util.concurrent.ThreadFactory; @@ -66,7 +66,7 @@ public class UpdateShardHandler implements SolrMetricProducer, SolrInfoBean { private final InstrumentedHttpRequestExecutor httpRequestExecutor; - private final Set metricNames = new HashSet<>(); + private final Set metricNames = ConcurrentHashMap.newKeySet(); private MetricRegistry registry; public UpdateShardHandler(UpdateShardHandlerConfig cfg) { diff --git a/solr/core/src/test/org/apache/solr/core/MockInfoBean.java b/solr/core/src/test/org/apache/solr/core/MockInfoBean.java index a4334bc6cf7..8ae35520b89 100644 --- a/solr/core/src/test/org/apache/solr/core/MockInfoBean.java +++ b/solr/core/src/test/org/apache/solr/core/MockInfoBean.java @@ -16,8 +16,8 @@ */ package org.apache.solr.core; -import java.util.HashSet; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import com.codahale.metrics.MetricRegistry; import org.apache.solr.metrics.MetricsMap; @@ -25,7 +25,7 @@ import org.apache.solr.metrics.SolrMetricManager; import org.apache.solr.metrics.SolrMetricProducer; class MockInfoBean implements SolrInfoBean, SolrMetricProducer { - Set metricNames = new HashSet<>(); + Set metricNames = ConcurrentHashMap.newKeySet(); MetricRegistry registry; @Override diff --git a/solr/core/src/test/org/apache/solr/handler/admin/MBeansHandlerTest.java b/solr/core/src/test/org/apache/solr/handler/admin/MBeansHandlerTest.java index 37ea5bd5183..76168857fde 100644 --- a/solr/core/src/test/org/apache/solr/handler/admin/MBeansHandlerTest.java +++ b/solr/core/src/test/org/apache/solr/handler/admin/MBeansHandlerTest.java @@ -19,14 +19,20 @@ package org.apache.solr.handler.admin; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.regex.Matcher; import java.util.regex.Pattern; +import com.codahale.metrics.MetricRegistry; import org.apache.solr.SolrTestCaseJ4; import org.apache.solr.common.params.CommonParams; import org.apache.solr.common.util.ContentStream; import org.apache.solr.common.util.ContentStreamBase; import org.apache.solr.common.util.NamedList; +import org.apache.solr.core.SolrInfoBean; import org.apache.solr.request.LocalSolrQueryRequest; import org.junit.BeforeClass; import org.junit.Test; @@ -120,4 +126,74 @@ public class MBeansHandlerTest extends SolrTestCaseJ4 { assertTrue("external entity ignored properly", true); } + + boolean runSnapshots; + + @Test + public void testMetricsSnapshot() throws Exception { + final CountDownLatch counter = new CountDownLatch(500); + MetricRegistry registry = new MetricRegistry(); + Set names = ConcurrentHashMap.newKeySet(); + SolrInfoBean bean = new SolrInfoBean() { + @Override + public String getName() { + return "foo"; + } + + @Override + public String getDescription() { + return "foo"; + } + + @Override + public Category getCategory() { + return Category.ADMIN; + } + + @Override + public Set getMetricNames() { + return names; + } + + @Override + public MetricRegistry getMetricRegistry() { + return registry; + } + }; + runSnapshots = true; + Thread modifier = new Thread(() -> { + int i = 0; + while (runSnapshots) { + bean.registerMetricName("name-" + i++); + try { + Thread.sleep(31); + } catch (InterruptedException e) { + runSnapshots = false; + break; + } + } + }); + Thread reader = new Thread(() -> { + while (runSnapshots) { + try { + bean.getMetricsSnapshot(); + } catch (Exception e) { + runSnapshots = false; + e.printStackTrace(); + fail("Exception getting metrics snapshot: " + e.toString()); + } + try { + Thread.sleep(53); + } catch (InterruptedException e) { + runSnapshots = false; + break; + } + counter.countDown(); + } + }); + modifier.start(); + reader.start(); + counter.await(30, TimeUnit.SECONDS); + runSnapshots = false; + } }