From a3624029ad262489b091d9a474430359f782d307 Mon Sep 17 00:00:00 2001 From: David Smiley Date: Tue, 28 Jul 2020 16:46:27 -0400 Subject: [PATCH] SOLR-14651: Metrics History could disable better (#1672) * SolrRrdBackendFactory should not be created if history is disabled * Disable MetricsHistoryHandler by default in tests * Await shutdown of all executors --- solr/CHANGES.txt | 3 +++ .../org/apache/solr/core/CoreContainer.java | 10 +++------ .../handler/admin/MetricsHistoryHandler.java | 21 ++++++------------- .../metrics/rrd/SolrRrdBackendFactory.java | 5 +++-- .../apache/solr/common/util/ExecutorUtil.java | 8 ++++++- .../org/apache/solr/util/TestHarness.java | 6 +++++- 6 files changed, 27 insertions(+), 26 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 30e76e898da..4f8a6138a05 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -131,6 +131,9 @@ Improvements * SOLR-13205: Prevent StringIndexOutOfBoundsException when parsing field names in SolrQueryParserBase (pramodkumar9 via Jason Gerlowski) +* SOLR-14651: The MetricsHistoryHandler can more completely disable itself when you tell it to. + Also, it now shuts down more thoroughly. (David Smiley) + Optimizations --------------------- diff --git a/solr/core/src/java/org/apache/solr/core/CoreContainer.java b/solr/core/src/java/org/apache/solr/core/CoreContainer.java index 1cec6e40bae..da95aab4a45 100644 --- a/solr/core/src/java/org/apache/solr/core/CoreContainer.java +++ b/solr/core/src/java/org/apache/solr/core/CoreContainer.java @@ -893,7 +893,7 @@ public class CoreContainer { Map initArgs; if (plugin != null && plugin.initArgs != null) { initArgs = plugin.initArgs.asMap(5); - initArgs.put(MetricsHistoryHandler.ENABLE_PROP, plugin.isEnabled()); + initArgs.putIfAbsent(MetricsHistoryHandler.ENABLE_PROP, plugin.isEnabled()); } else { initArgs = new HashMap<>(); } @@ -919,12 +919,8 @@ public class CoreContainer { } }; // enable local metrics unless specifically set otherwise - if (!initArgs.containsKey(MetricsHistoryHandler.ENABLE_NODES_PROP)) { - initArgs.put(MetricsHistoryHandler.ENABLE_NODES_PROP, true); - } - if (!initArgs.containsKey(MetricsHistoryHandler.ENABLE_REPLICAS_PROP)) { - initArgs.put(MetricsHistoryHandler.ENABLE_REPLICAS_PROP, true); - } + initArgs.putIfAbsent(MetricsHistoryHandler.ENABLE_NODES_PROP, true); + initArgs.putIfAbsent(MetricsHistoryHandler.ENABLE_REPLICAS_PROP, true); } metricsHistoryHandler = new MetricsHistoryHandler(name, metricsHandler, client, cloudManager, initArgs); diff --git a/solr/core/src/java/org/apache/solr/handler/admin/MetricsHistoryHandler.java b/solr/core/src/java/org/apache/solr/handler/admin/MetricsHistoryHandler.java index 8173f4534ea..56c40031a49 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/MetricsHistoryHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/admin/MetricsHistoryHandler.java @@ -207,8 +207,6 @@ public class MetricsHistoryHandler extends RequestHandlerBase implements Permiss this.metricsHandler = metricsHandler; this.cloudManager = cloudManager; this.timeSource = cloudManager != null ? cloudManager.getTimeSource() : TimeSource.NANO_TIME; - factory = new SolrRrdBackendFactory(solrClient, CollectionAdminParams.SYSTEM_COLL, - syncPeriod, this.timeSource); counters.put(Group.core.toString(), DEFAULT_CORE_COUNTERS); counters.put(Group.node.toString(), Collections.emptyList()); @@ -228,6 +226,9 @@ public class MetricsHistoryHandler extends RequestHandlerBase implements Permiss } if (enable) { + factory = new SolrRrdBackendFactory(solrClient, CollectionAdminParams.SYSTEM_COLL, + syncPeriod, this.timeSource); + collectService = (ScheduledThreadPoolExecutor) Executors.newScheduledThreadPool(1, new SolrNamedThreadFactory("MetricsHistoryHandler")); collectService.setRemoveOnCancelPolicy(true); @@ -237,6 +238,8 @@ public class MetricsHistoryHandler extends RequestHandlerBase implements Permiss timeSource.convertDelay(TimeUnit.SECONDS, collectPeriod, TimeUnit.MILLISECONDS), TimeUnit.MILLISECONDS); checkSystemCollection(); + } else { + factory = null; } } @@ -654,19 +657,7 @@ public class MetricsHistoryHandler extends RequestHandlerBase implements Permiss if (log.isDebugEnabled()) { log.debug("Closing {}", hashCode()); } - if (collectService != null) { - boolean shutdown = false; - while (!shutdown) { - try { - // Wait a while for existing tasks to terminate - collectService.shutdownNow(); - shutdown = collectService.awaitTermination(5, TimeUnit.SECONDS); - } catch (InterruptedException ie) { - // Preserve interrupt status - Thread.currentThread().interrupt(); - } - } - } + ExecutorUtil.shutdownNowAndAwaitTermination(collectService); if (factory != null) { factory.close(); } diff --git a/solr/core/src/java/org/apache/solr/metrics/rrd/SolrRrdBackendFactory.java b/solr/core/src/java/org/apache/solr/metrics/rrd/SolrRrdBackendFactory.java index 97d28f119ea..105fd64ce06 100644 --- a/solr/core/src/java/org/apache/solr/metrics/rrd/SolrRrdBackendFactory.java +++ b/solr/core/src/java/org/apache/solr/metrics/rrd/SolrRrdBackendFactory.java @@ -44,6 +44,7 @@ import org.apache.solr.common.SolrInputDocument; import org.apache.solr.common.params.CollectionAdminParams; import org.apache.solr.common.params.CommonParams; import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.solr.common.util.ExecutorUtil; import org.apache.solr.common.util.IOUtils; import org.apache.solr.common.util.Pair; import org.apache.solr.common.util.TimeSource; @@ -459,9 +460,9 @@ public class SolrRrdBackendFactory extends RrdBackendFactory implements SolrClos log.debug("Closing {}", hashCode()); } closed = true; - backends.forEach((p, b) -> IOUtils.closeQuietly(b)); + backends.values().forEach(IOUtils::closeQuietly); backends.clear(); - syncService.shutdownNow(); + ExecutorUtil.shutdownNowAndAwaitTermination(syncService); syncService = null; } } diff --git a/solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java b/solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java index 670a6b42829..6664dbf02b3 100644 --- a/solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java +++ b/solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java @@ -72,11 +72,17 @@ public class ExecutorUtil { } public static void shutdownAndAwaitTermination(ExecutorService pool) { - if(pool == null) return; + if (pool == null) return; pool.shutdown(); // Disable new tasks from being submitted awaitTermination(pool); } + public static void shutdownNowAndAwaitTermination(ExecutorService pool) { + if (pool == null) return; + pool.shutdownNow(); // Disable new tasks from being submitted; interrupt existing tasks + awaitTermination(pool); + } + public static void awaitTermination(ExecutorService pool) { boolean shutdown = false; while (!shutdown) { diff --git a/solr/test-framework/src/java/org/apache/solr/util/TestHarness.java b/solr/test-framework/src/java/org/apache/solr/util/TestHarness.java index 5814ad42fda..7657c3f8dd1 100644 --- a/solr/test-framework/src/java/org/apache/solr/util/TestHarness.java +++ b/solr/test-framework/src/java/org/apache/solr/util/TestHarness.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.io.StringWriter; import java.nio.charset.StandardCharsets; import java.nio.file.Path; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -191,7 +192,10 @@ public class TestHarness extends BaseTestHarness { attributes.put("class", SolrJmxReporter.class.getName()); PluginInfo defaultPlugin = new PluginInfo("reporter", attributes); MetricsConfig metricsConfig = new MetricsConfig.MetricsConfigBuilder() - .setMetricReporterPlugins(new PluginInfo[] {defaultPlugin}) + .setMetricReporterPlugins(new PluginInfo[]{defaultPlugin}) + .setHistoryHandler( + Boolean.getBoolean("metricsHistory") + ? null : new PluginInfo("typeUnused", Collections.singletonMap("enable", "false"))) .build(); return new NodeConfig.NodeConfigBuilder("testNode", solrHome)