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

@@ -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
 ---------------------
 

solr/core/src/java/org/apache/solr/core/CoreContainer.java

@@ -893,7 +893,7 @@ public class CoreContainer {
     Map<String, Object> 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.putIfAbsent(MetricsHistoryHandler.ENABLE_NODES_PROP, true);
-        initArgs.put(MetricsHistoryHandler.ENABLE_NODES_PROP, true);
+      initArgs.putIfAbsent(MetricsHistoryHandler.ENABLE_REPLICAS_PROP, true);
-      }
-      if (!initArgs.containsKey(MetricsHistoryHandler.ENABLE_REPLICAS_PROP)) {
-        initArgs.put(MetricsHistoryHandler.ENABLE_REPLICAS_PROP, true);
-      }
     }
     metricsHistoryHandler = new MetricsHistoryHandler(name, metricsHandler,
         client, cloudManager, initArgs);

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) {
+    ExecutorUtil.shutdownNowAndAwaitTermination(collectService);
-      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();
-        }
-      }
-    }
     if (factory != null) {
       factory.close();
     }

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;
   }
 }

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) {

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)