#156 - Pretty much no tests use MetricsHistoryHandler.

apache · Jul 14, 2020 · a7b9847 · a7b9847
1 parent 883c3fa
commit a7b9847
Show file tree

Hide file tree

Showing 6 changed files with 27 additions and 47 deletions.
diff --git a/solr/core/src/java/org/apache/solr/core/CoreContainer.java b/solr/core/src/java/org/apache/solr/core/CoreContainer.java
@@ -781,9 +781,11 @@ public void load() {
         metricsHandler.initializeMetrics(solrMetricsContext, METRICS_PATH);
       });
 
-      work.collect(() -> {
-        createMetricsHistoryHandler();
-      });
+      if (!Boolean.getBoolean("solr.disableMetricsHistoryHandler")) {
+        work.collect(() -> {
+          createMetricsHistoryHandler();
+        });
+      }
 
       work.collect(() -> {
         autoscalingHistoryHandler = createHandler(AUTOSCALING_HISTORY_PATH, AutoscalingHistoryHandler.class.getName(), AutoscalingHistoryHandler.class);

diff --git a/solr/core/src/java/org/apache/solr/core/SolrCore.java b/solr/core/src/java/org/apache/solr/core/SolrCore.java
@@ -1549,11 +1549,6 @@ public void open() {
     MDCLoggingContext.setCore(this);
   }
 
-  @Override
-  public void close() {
-    close(false);
-  }
-
   /**
    * Close all resources allocated by the core if it is no longer in use...
    * <ul>
@@ -1579,8 +1574,8 @@ public void close() {
    *
    * @see #isClosed()
    */
-
-  public void close(boolean failedInConstructor) {
+  @Override
+  public void close() {
     int count = refCount.decrementAndGet();
     if (count > 0) return; // close is called often, and only actually closes if nothing is using it.
     if (count < 0) {
@@ -1600,9 +1595,6 @@ public void close(boolean failedInConstructor) {
         }
       }
 
-
-
-
       List<Callable<Object>> closeHookCalls = new ArrayList<>();
 
       if (closeHooks != null) {
@@ -1616,9 +1608,6 @@ public void close(boolean failedInConstructor) {
 
       assert ObjectReleaseTracker.release(searcherExecutor);
 
-    //  closer.add("PreCloseHooks", closeHookCalls);
-
-
       List<Callable<Object>> closeCalls = new ArrayList<Callable<Object>>();
       closeCalls.addAll(closeHookCalls);
       closeCalls.add(() -> {
@@ -1658,34 +1647,18 @@ public void close(boolean failedInConstructor) {
 
       AtomicBoolean coreStateClosed = new AtomicBoolean(false);
 
-      if (!failedInConstructor) {
-        closer.add("SolrCoreState", () -> {
-          boolean closed = false;
-          if (updateHandler != null && updateHandler instanceof IndexWriterCloser) {
-            closed = solrCoreState.decrefSolrCoreState((IndexWriterCloser) updateHandler);
-          } else {
-            closed = solrCoreState.decrefSolrCoreState(null);
-          }
-          coreStateClosed.set(closed);
-          return solrCoreState;
-        });
-      }
+      closer.add("SolrCoreState", () -> {
+        boolean closed = false;
+        if (updateHandler != null && updateHandler instanceof IndexWriterCloser) {
+          closed = solrCoreState.decrefSolrCoreState((IndexWriterCloser) updateHandler);
+        } else {
+          closed = solrCoreState.decrefSolrCoreState(null);
+        }
+        coreStateClosed.set(closed);
+        return solrCoreState;
+      });
 
       closer.add("coreAsyncTaskExecutor", coreAsyncTaskExecutor, () -> {
-        // Since we waited for the searcherExecutor to shut down,
-        // there should be no more searchers warming in the background
-        // that we need to take care of.
-        //
-        // For the case that a searcher was registered *before* warming
-        // then the searchExecutor will throw an exception when getSearcher()
-        // tries to use it, and the exception handling code should close it.
-// nocommit
-//        synchronized (searcherLock) {
-//          for (RefCounted<SolrIndexSearcher> searcher :  _searchers) {
-//            searcher.decref();
-//          }
-//        }
-
 
         return "Searcher";
       });
@@ -1705,11 +1678,11 @@ public void close(boolean failedInConstructor) {
 
       });
 
-      closer.add("closeSearcher", ()->{
+      closer.add("closeSearcher", () -> {
         closeSearcher();
       });
 
-      closer.add("ClearInfoReg&ReleaseSnapShotsDir", ()->{
+      closer.add("ClearInfoReg&ReleaseSnapShotsDir", () -> {
         closeSearcher();
         return "searcher";
       }, () -> {

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
@@ -665,11 +665,12 @@ public void close() {
     if (log.isDebugEnabled()) {
       log.debug("Closing {}", hashCode());
     }
-    collectService.shutdownNow();
+
     try (ParWork closer = new ParWork(this)) {
-      closer.collect(collectService);
       closer.collect(factory);
-      closer.addCollect("metricsHistoryHandlerClose");
+      closer.addCollect("factory");
+      closer.collect(collectService);
+      closer.addCollect("collectService");
     }
 
     knownDbs.clear();

diff --git a/solr/core/src/test/org/apache/solr/cloud/MetricsHistoryWithAuthIntegrationTest.java b/solr/core/src/test/org/apache/solr/cloud/MetricsHistoryWithAuthIntegrationTest.java
@@ -61,6 +61,7 @@ public class MetricsHistoryWithAuthIntegrationTest extends SolrCloudTestCase {
 
   @BeforeClass
   public static void setupCluster() throws Exception {
+    System.setProperty("solr.disableMetricsHistoryHandler", "false");
     System.setProperty("solr.disableJmxReporter", "false");
     String solrXml = MiniSolrCloudCluster.DEFAULT_CLOUD_SOLR_XML.replace("<metrics>\n",
         "<metrics>\n" + SOLR_XML_HISTORY_CONFIG);

diff --git a/solr/core/src/test/org/apache/solr/metrics/SolrCoreMetricManagerTest.java b/solr/core/src/test/org/apache/solr/metrics/SolrCoreMetricManagerTest.java
@@ -44,6 +44,7 @@ public class SolrCoreMetricManagerTest extends SolrTestCaseJ4 {
 
   @Before
   public void beforeTest() throws Exception {
+    useFactory(null);
     initCore("solrconfig-basic.xml", "schema.xml");
     coreMetricManager = h.getCore().getCoreMetricManager();
     metricManager = h.getCore().getCoreContainer().getMetricManager();

diff --git a/solr/test-framework/src/java/org/apache/solr/SolrTestCase.java b/solr/test-framework/src/java/org/apache/solr/SolrTestCase.java
@@ -194,6 +194,8 @@ public static void setDefaultConfigDirSysPropIfNotSet() throws Exception {
       // nocommit - not used again yet
       System.setProperty("solr.OverseerStateUpdateDelay", "0");
 
+      System.setProperty("solr.disableMetricsHistoryHandler", "true");
+
       System.setProperty("solr.leaderThrottle", "0");
       System.setProperty("solr.recoveryThrottle", "0");