SOLR-13677: All Metrics Gauges should be unregistered by the objects …

…that registered them (#836)

* SOLR-13677: All Metrics Gauges should be unregistered by the objects that registered them

solr/contrib/dataimporthandler/src/java/org/apache/solr/handler/dataimport/DataImportHandler.java

@@ -36,7 +36,7 @@
 import org.apache.solr.core.SolrResourceLoader;
 import org.apache.solr.handler.RequestHandlerBase;
 import org.apache.solr.metrics.MetricsMap;
-import org.apache.solr.metrics.SolrMetricManager;
+import org.apache.solr.metrics.SolrMetrics;
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.response.RawResponseWriter;
 import org.apache.solr.response.SolrQueryResponse;
@@ -275,8 +275,8 @@ public boolean upload(SolrInputDocument document) {
   }
 
   @Override
-  public void initializeMetrics(SolrMetricManager manager, String registryName, String tag, String scope) {
-    super.initializeMetrics(manager, registryName, tag, scope);
+  public void initializeMetrics(SolrMetrics m) {
+    super.initializeMetrics(m);
     metrics = new MetricsMap((detailed, map) -> {
       if (importer != null) {
         DocBuilder.Statistics cumulative = importer.cumulativeStatistics;
@@ -299,7 +299,7 @@ public void initializeMetrics(SolrMetricManager manager, String registryName, St
         map.put(DataImporter.MSG.TOTAL_DOCS_SKIPPED, cumulative.skipDocCount);
       }
     });
-    manager.registerGauge(this, registryName, metrics, tag, true, "importer", getCategory().toString(), scope);
+    solrMetrics.gauge(this, metrics, true, "importer", getCategory().toString());
   }
 
   // //////////////////////SolrInfoMBeans methods //////////////////////

solr/core/src/java/org/apache/solr/cloud/ReplicateFromLeader.java

@@ -133,7 +133,7 @@ private static String toPollIntervalStr(int ms) {
 
   public void stopReplication() {
     if (replicationProcess != null) {
-      replicationProcess.close();
+      replicationProcess.shutdown();
     }
   }
 }

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

@@ -195,10 +195,11 @@ public T put(String name, T plugin) {
     PluginHolder<T> pluginHolder = new PluginHolder<>(null, plugin);
     pluginHolder.registerAPI = false;
     PluginHolder<T> old = put(name, pluginHolder);
+    if(old != null)  closeQuietly(old);
     return old == null ? null : old.get();
   }
 
-  PluginHolder<T> put(String name, PluginHolder<T> plugin) {
+  public PluginHolder<T> put(String name, PluginHolder<T> plugin) {
     Boolean registerApi = null;
     Boolean disableHandler = null;
     if (plugin.pluginInfo != null) {

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

@@ -233,7 +233,7 @@ public final class SolrCore implements SolrInfoBean, SolrMetricProducer, Closeab
   private final CoreContainer coreContainer;
 
   private Set<String> metricNames = ConcurrentHashMap.newKeySet();
-  private String metricTag = Integer.toHexString(hashCode());
+  private final String metricTag = getUniqueMetricTag(null);
 
   public volatile boolean searchEnabled = true;
   public volatile boolean indexEnabled = true;

solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java

@@ -92,7 +92,7 @@
 import org.apache.solr.core.backup.repository.LocalFileSystemRepository;
 import org.apache.solr.handler.IndexFetcher.IndexFetchResult;
 import org.apache.solr.metrics.MetricsMap;
-import org.apache.solr.metrics.SolrMetricManager;
+import org.apache.solr.metrics.SolrMetrics;
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.response.SolrQueryResponse;
 import org.apache.solr.search.SolrIndexSearcher;
@@ -863,21 +863,20 @@ private CommitVersionInfo getIndexVersion() {
   }
 
   @Override
-  public void initializeMetrics(SolrMetricManager manager, String registry, String tag, String scope) {
-    super.initializeMetrics(manager, registry, tag, scope);
-
-    manager.registerGauge(this, registry, () -> (core != null && !core.isClosed() ? NumberUtils.readableSize(core.getIndexSize()) : ""),
-        tag, true, "indexSize", getCategory().toString(), scope);
-    manager.registerGauge(this, registry, () -> (core != null && !core.isClosed() ? getIndexVersion().toString() : ""),
-        tag, true, "indexVersion", getCategory().toString(), scope);
-    manager.registerGauge(this, registry, () -> (core != null && !core.isClosed() ? getIndexVersion().generation : 0),
-        tag, true, GENERATION, getCategory().toString(), scope);
-    manager.registerGauge(this, registry, () -> (core != null && !core.isClosed() ? core.getIndexDir() : ""),
-        tag, true, "indexPath", getCategory().toString(), scope);
-    manager.registerGauge(this, registry, () -> isMaster,
-        tag, true, "isMaster", getCategory().toString(), scope);
-    manager.registerGauge(this, registry, () -> isSlave,
-        tag, true, "isSlave", getCategory().toString(), scope);
+  public void initializeMetrics(SolrMetrics m) {
+    super.initializeMetrics(m);
+    solrMetrics.gauge(this,  () -> (core != null && !core.isClosed() ? NumberUtils.readableSize(core.getIndexSize()) : ""),
+        true, "indexSize", getCategory().toString());
+    solrMetrics.gauge(this, () -> (core != null && !core.isClosed() ? getIndexVersion().toString() : ""),
+         true, "indexVersion", getCategory().toString());
+    solrMetrics.gauge(this, () -> (core != null && !core.isClosed() ? getIndexVersion().generation : 0),
+        true, GENERATION, getCategory().toString());
+    solrMetrics.gauge(this, () -> (core != null && !core.isClosed() ? core.getIndexDir() : ""),
+        true, "indexPath", getCategory().toString());
+    solrMetrics.gauge(this, () -> isMaster,
+         true, "isMaster", getCategory().toString());
+    solrMetrics.gauge(this, () -> isSlave,
+         true, "isSlave", getCategory().toString());
     final MetricsMap fetcherMap = new MetricsMap((detailed, map) -> {
       IndexFetcher fetcher = currentIndexFetcher;
       if (fetcher != null) {
@@ -906,13 +905,13 @@ public void initializeMetrics(SolrMetricManager manager, String registry, String
         addVal(map, IndexFetcher.CONF_FILES_REPLICATED, props, String.class);
       }
     });
-    manager.registerGauge(this, registry, fetcherMap, tag, true, "fetcher", getCategory().toString(), scope);
-    manager.registerGauge(this, registry, () -> isMaster && includeConfFiles != null ? includeConfFiles : "",
-        tag, true, "confFilesToReplicate", getCategory().toString(), scope);
-    manager.registerGauge(this, registry, () -> isMaster ? getReplicateAfterStrings() : Collections.<String>emptyList(),
-        tag, true, REPLICATE_AFTER, getCategory().toString(), scope);
-    manager.registerGauge(this, registry, () -> isMaster && replicationEnabled.get(),
-        tag, true, "replicationEnabled", getCategory().toString(), scope);
+    solrMetrics.gauge(this , fetcherMap, true, "fetcher", getCategory().toString());
+    solrMetrics.gauge(this, () -> isMaster && includeConfFiles != null ? includeConfFiles : "",
+         true, "confFilesToReplicate", getCategory().toString());
+    solrMetrics.gauge(this, () -> isMaster ? getReplicateAfterStrings() : Collections.<String>emptyList(),
+        true, REPLICATE_AFTER, getCategory().toString());
+    solrMetrics.gauge(this,  () -> isMaster && replicationEnabled.get(),
+        true, "replicationEnabled", getCategory().toString());
   }
 
   //TODO Should a failure retrieving any piece of info mark the overall request as a failure?  Is there a core set of values that are required to make a response here useful?
@@ -1387,7 +1386,7 @@ public void postClose(SolrCore core) {}
     });
   }
 
-  public void close() {
+  public void shutdown() {
     if (executorService != null) executorService.shutdown();
     if (pollingIndexFetcher != null) {
       pollingIndexFetcher.destroy();

solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java

@@ -22,11 +22,14 @@
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 
-import com.codahale.metrics.MetricRegistry;
-import com.google.common.collect.ImmutableList;
 import com.codahale.metrics.Counter;
 import com.codahale.metrics.Meter;
+import com.codahale.metrics.MetricRegistry;
 import com.codahale.metrics.Timer;
+import com.google.common.collect.ImmutableList;
+import org.apache.solr.api.Api;
+import org.apache.solr.api.ApiBag;
+import org.apache.solr.api.ApiSupport;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.params.ShardParams;
 import org.apache.solr.common.params.SolrParams;
@@ -36,16 +39,13 @@
 import org.apache.solr.core.PluginInfo;
 import org.apache.solr.core.SolrInfoBean;
 import org.apache.solr.metrics.MetricsMap;
-import org.apache.solr.metrics.SolrMetricManager;
 import org.apache.solr.metrics.SolrMetricProducer;
+import org.apache.solr.metrics.SolrMetrics;
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.request.SolrRequestHandler;
 import org.apache.solr.response.SolrQueryResponse;
 import org.apache.solr.search.SyntaxError;
 import org.apache.solr.util.SolrPluginUtils;
-import org.apache.solr.api.Api;
-import org.apache.solr.api.ApiBag;
-import org.apache.solr.api.ApiSupport;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -79,9 +79,6 @@ public abstract class RequestHandlerBase implements SolrRequestHandler, SolrInfo
   private PluginInfo pluginInfo;
 
   private Set<String> metricNames = ConcurrentHashMap.newKeySet();
-  private MetricRegistry registry;
-  protected String registryName;
-  protected SolrMetricManager metricManager;
 
 
   @SuppressForbidden(reason = "Need currentTimeMillis, used only for stats output")
@@ -143,22 +140,27 @@ public void init(NamedList args) {
 
   }
 
+  protected SolrMetrics solrMetrics;
+
+  @Override
+  public SolrMetrics getMetrics() {
+    return solrMetrics;
+  }
+
   @Override
-  public void initializeMetrics(SolrMetricManager manager, String registryName, String tag, final String scope) {
-    this.metricManager = manager;
-    this.registryName = registryName;
-    this.registry = manager.registry(registryName);
-    numErrors = manager.meter(this, registryName, "errors", getCategory().toString(), scope);
-    numServerErrors = manager.meter(this, registryName, "serverErrors", getCategory().toString(), scope);
-    numClientErrors = manager.meter(this, registryName, "clientErrors", getCategory().toString(), scope);
-    numTimeouts = manager.meter(this, registryName, "timeouts", getCategory().toString(), scope);
-    requests = manager.counter(this, registryName, "requests", getCategory().toString(), scope);
+  public void initializeMetrics(SolrMetrics m) {
+    this.solrMetrics = m.getChildInfo(this);
+    numErrors = solrMetrics.meter(this, "errors", getCategory().toString());
+    numServerErrors = solrMetrics.meter(this, "serverErrors", getCategory().toString());
+    numClientErrors = solrMetrics.meter(this, "clientErrors", getCategory().toString());
+    numTimeouts = solrMetrics.meter(this, "timeouts", getCategory().toString());
+    requests = solrMetrics.counter(this, "requests", getCategory().toString());
     MetricsMap metricsMap = new MetricsMap((detail, map) ->
         shardPurposes.forEach((k, v) -> map.put(k, v.getCount())));
-    manager.registerGauge(this, registryName, metricsMap, tag, true, "shardRequests", getCategory().toString(), scope);
-    requestTimes = manager.timer(this, registryName, "requestTimes", getCategory().toString(), scope);
-    totalTime = manager.counter(this, registryName, "totalTime", getCategory().toString(), scope);
-    manager.registerGauge(this, registryName, () -> handlerStart, tag, true, "handlerStart", getCategory().toString(), scope);
+    solrMetrics.gauge(this, metricsMap, true, "shardRequests", getCategory().toString());
+    requestTimes = solrMetrics.timer(this,"requestTimes", getCategory().toString());
+    totalTime = solrMetrics.counter(this, "totalTime", getCategory().toString());
+    solrMetrics.gauge(this, () -> handlerStart, true, "handlerStart", getCategory().toString());
   }
 
   public static SolrParams getSolrParamsFromNamedList(NamedList args, String key) {
@@ -274,7 +276,7 @@ public Set<String> getMetricNames() {
 
   @Override
   public MetricRegistry getMetricRegistry() {
-    return registry;
+    return solrMetrics.getRegistry();
   }
 
   @Override

solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java

@@ -46,6 +46,7 @@
 import org.apache.solr.handler.RequestHandlerBase;
 import org.apache.solr.logging.MDCLoggingContext;
 import org.apache.solr.metrics.SolrMetricManager;
+import org.apache.solr.metrics.SolrMetrics;
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.response.SolrQueryResponse;
 import org.apache.solr.security.AuthorizationContext;
@@ -120,10 +121,10 @@ final public void init(NamedList args) {
   }
 
   @Override
-  public void initializeMetrics(SolrMetricManager manager, String registryName, String tag, String scope) {
-    super.initializeMetrics(manager, registryName, tag, scope);
-    parallelExecutor = MetricUtils.instrumentedExecutorService(parallelExecutor, this, manager.registry(registryName),
-        SolrMetricManager.mkName("parallelCoreAdminExecutor", getCategory().name(),scope, "threadPool"));
+  public void initializeMetrics(SolrMetrics m) {
+    super.initializeMetrics(m);
+    parallelExecutor = MetricUtils.instrumentedExecutorService(parallelExecutor, this, solrMetrics.getRegistry(),
+        SolrMetricManager.mkName("parallelCoreAdminExecutor", getCategory().name(), solrMetrics.scope, "threadPool"));
   }
   @Override
   public Boolean registerV2() {

solr/core/src/java/org/apache/solr/handler/component/SuggestComponent.java

@@ -48,8 +48,8 @@
 import org.apache.solr.core.SolrCore;
 import org.apache.solr.core.SolrEventListener;
 import org.apache.solr.metrics.MetricsMap;
-import org.apache.solr.metrics.SolrMetricManager;
 import org.apache.solr.metrics.SolrMetricProducer;
+import org.apache.solr.metrics.SolrMetrics;
 import org.apache.solr.search.SolrIndexSearcher;
 import org.apache.solr.spelling.suggest.SolrSuggester;
 import org.apache.solr.spelling.suggest.SuggesterOptions;
@@ -88,9 +88,6 @@ public class SuggestComponent extends SearchComponent implements SolrCoreAware,
   @SuppressWarnings("unchecked")
   protected NamedList initParams;
 
-  protected SolrMetricManager metricManager;
-  protected String registryName;
-
   /**
    * Key is the dictionary name used in SolrConfig, value is the corresponding {@link SolrSuggester}
    */
@@ -350,19 +347,25 @@ public String getDescription() {
     return "Suggester component";
   }
 
+  protected SolrMetrics metricsInfo;
+
+  @Override
+  public SolrMetrics getMetrics() {
+    return metricsInfo;
+  }
+
   @Override
-  public void initializeMetrics(SolrMetricManager manager, String registryName, String tag, String scope) {
-    this.registryName = registryName;
-    this.metricManager = manager;
-    registry = manager.registry(registryName);
-    manager.registerGauge(this, registryName, () -> ramBytesUsed(), tag, true, "totalSizeInBytes", getCategory().toString(), scope);
+  public void initializeMetrics(SolrMetrics info) {
+    this.metricsInfo = info.getChildInfo(this);
+
+    metricsInfo.metricManager.registerGauge(this, info.registry, () -> ramBytesUsed(), metricsInfo.tag, true, "totalSizeInBytes", getCategory().toString(), metricsInfo.scope);
     MetricsMap suggestersMap = new MetricsMap((detailed, map) -> {
       for (Map.Entry<String, SolrSuggester> entry : suggesters.entrySet()) {
         SolrSuggester suggester = entry.getValue();
         map.put(entry.getKey(), suggester.toString());
       }
     });
-    manager.registerGauge(this, registryName, suggestersMap, tag, true, "suggesters", getCategory().toString(), scope);
+    metricsInfo.metricManager.registerGauge(this, metricsInfo.registry, suggestersMap, metricsInfo.tag, true, "suggesters", getCategory().toString(), metricsInfo.scope);
   }
 
   @Override