SOLR-10671: Add abstract doInit method to the SolrMetricReporter base class.

(Christine Poerschke, Anshum Gupta)

solr/CHANGES.txt

@@ -89,6 +89,9 @@ Upgrading from Solr 6.x
 * In solrconfig.xml the deprecated <mergePolicy> and <mergeFactor> and <maxMergeDocs> elements have
   been removed in favor of the <mergePolicyFactory> element introduced by SOLR-8621 in Solr 5.5.0.
 
+* Custom SolrMetricReporter classes must implement a new, factored out doInit() method and the validate() method
+  must pass for all reporters including reporters that are not enabled, please see SOLR-10671 for details.
+
 New Features
 ----------------------
 * SOLR-9857, SOLR-9858: Collect aggregated metrics from nodes and shard leaders in overseer. (ab)
@@ -230,6 +233,9 @@ Other Changes
 
 * SOLR-10801: Remove several deprecated methods that were exposed to plugin writers (hossman)
 
+* SOLR-10671: Add abstract doInit method to the SolrMetricReporter base class.
+  (Christine Poerschke, Anshum Gupta)
+
 ==================  6.7.0 ==================
 
 Consult the LUCENE_CHANGES.txt file for additional, low level, changes in this release.

solr/core/src/java/org/apache/solr/metrics/SolrMetricReporter.java

@@ -17,16 +17,21 @@
 package org.apache.solr.metrics;
 
 import java.io.Closeable;
+import java.lang.invoke.MethodHandles;
 
 import org.apache.solr.core.PluginInfo;
 import org.apache.solr.util.SolrPluginUtils;
 import org.apache.solr.util.plugin.PluginInfoInitialized;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Interface for 'pluggable' metric reporters.
  */
 public abstract class SolrMetricReporter implements Closeable, PluginInfoInitialized {
 
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
   protected final String registryName;
   protected final SolrMetricManager metricManager;
   protected PluginInfo pluginInfo;
@@ -56,12 +61,23 @@ public abstract class SolrMetricReporter implements Closeable, PluginInfoInitial
       }
     }
     validate();
+    if (!enabled) {
+      log.info("Reporter disabled for registry " + registryName);
+      return;
+    }
+    log.debug("Initializing for registry " + registryName);
+    doInit();
   }
 
   /**
-   * Enable reporting, defaults to true. Implementations should check this flag in
+   * Reporter initialization implementation.
-   * {@link #validate()} and accordingly enable or disable reporting.
+   */
-   * @param enabled enable, defaults to true when null or not set.
+  protected abstract void doInit();
+
+  /**
+   * Enable reporting, defaults to true. {@link #init(PluginInfo)} checks
+   * this flag before calling {@link #doInit()} to initialize reporting.
+   * @param enabled - whether or not reporting is to be enabled
    */
   public void setEnabled(Boolean enabled) {
     if (enabled != null) {

solr/core/src/java/org/apache/solr/metrics/reporters/SolrGangliaReporter.java

@@ -110,11 +110,14 @@ public class SolrGangliaReporter extends SolrMetricReporter {
   }
 
   @Override
-  protected void validate() throws IllegalStateException {
+  protected void doInit() {
-    if (!enabled) {
+    if (!testing) {
-      log.info("Reporter disabled for registry " + registryName);
+      start();
-      return;
     }
+  }
+
+  @Override
+  protected void validate() throws IllegalStateException {
     if (host == null) {
       throw new IllegalStateException("Init argument 'host' must be set to a valid Ganglia server name.");
     }
@@ -124,9 +127,6 @@ public class SolrGangliaReporter extends SolrMetricReporter {
     if (period < 1) {
       throw new IllegalStateException("Init argument 'period' is in time unit 'seconds' and must be at least 1.");
     }
-    if (!testing) {
-      start();
-    }
   }
 
   //this is a separate method for unit tests

solr/core/src/java/org/apache/solr/metrics/reporters/SolrGraphiteReporter.java

@@ -99,23 +99,10 @@ public class SolrGraphiteReporter extends SolrMetricReporter {
   }
 
   @Override
-  protected void validate() throws IllegalStateException {
+  protected void doInit() {
-    if (!enabled) {
-      log.info("Reporter disabled for registry " + registryName);
-      return;
-    }
-    if (host == null) {
-      throw new IllegalStateException("Init argument 'host' must be set to a valid Graphite server name.");
-    }
-    if (port == -1) {
-      throw new IllegalStateException("Init argument 'port' must be set to a valid Graphite server port.");
-    }
     if (reporter != null) {
       throw new IllegalStateException("Already started once?");
     }
-    if (period < 1) {
-      throw new IllegalStateException("Init argument 'period' is in time unit 'seconds' and must be at least 1.");
-    }
     GraphiteSender graphite;
     String id = host + ":" + port + ":" + pickled;
     graphite = serviceRegistry.getOrCreate(id, () -> {
@@ -146,6 +133,19 @@ public class SolrGraphiteReporter extends SolrMetricReporter {
     reporter.start(period, TimeUnit.SECONDS);
   }
 
+  @Override
+  protected void validate() throws IllegalStateException {
+    if (host == null) {
+      throw new IllegalStateException("Init argument 'host' must be set to a valid Graphite server name.");
+    }
+    if (port == -1) {
+      throw new IllegalStateException("Init argument 'port' must be set to a valid Graphite server port.");
+    }
+    if (period < 1) {
+      throw new IllegalStateException("Init argument 'period' is in time unit 'seconds' and must be at least 1.");
+    }
+  }
+
   @Override
   public void close() throws IOException {
     if (reporter != null) {

solr/core/src/java/org/apache/solr/metrics/reporters/SolrJmxReporter.java

@@ -33,7 +33,6 @@ import com.codahale.metrics.JmxReporter;
 import com.codahale.metrics.MetricFilter;
 import com.codahale.metrics.MetricRegistry;
 import com.codahale.metrics.MetricRegistryListener;
-import org.apache.solr.core.PluginInfo;
 import org.apache.solr.metrics.MetricsMap;
 import org.apache.solr.metrics.SolrMetricManager;
 import org.apache.solr.metrics.SolrMetricReporter;
@@ -74,20 +73,7 @@ public class SolrJmxReporter extends SolrMetricReporter {
     setDomain(registryName);
   }
 
-  /**
+  protected synchronized void doInit() {
-   * Initializes the reporter by finding an MBeanServer
-   * and registering the metricManager's metric registry.
-   *
-   * @param pluginInfo the configuration for the reporter
-   */
-  @Override
-  public synchronized void init(PluginInfo pluginInfo) {
-    super.init(pluginInfo);
-    if (!enabled) {
-      log.info("Reporter disabled for registry " + registryName);
-      return;
-    }
-    log.debug("Initializing for registry " + registryName);
     if (serviceUrl != null && agentId != null) {
       mBeanServer = JmxUtil.findFirstMBeanServer();
       log.warn("No more than one of serviceUrl({}) and agentId({}) should be configured, using first MBeanServer instead of configuration.",

solr/core/src/java/org/apache/solr/metrics/reporters/SolrSlf4jReporter.java

@@ -95,14 +95,7 @@ public class SolrSlf4jReporter extends SolrMetricReporter {
   }
 
   @Override
-  protected void validate() throws IllegalStateException {
+  protected void doInit() {
-    if (!enabled) {
-      log.info("Reporter disabled for registry " + registryName);
-      return;
-    }
-    if (period < 1) {
-      throw new IllegalStateException("Init argument 'period' is in time unit 'seconds' and must be at least 1.");
-    }
     if (instancePrefix == null) {
       instancePrefix = registryName;
     } else {
@@ -139,6 +132,13 @@ public class SolrSlf4jReporter extends SolrMetricReporter {
     reporter.start(period, TimeUnit.SECONDS);
   }
 
+  @Override
+  protected void validate() throws IllegalStateException {
+    if (period < 1) {
+      throw new IllegalStateException("Init argument 'period' is in time unit 'seconds' and must be at least 1.");
+    }
+  }
+
   @Override
   public void close() throws IOException {
     if (reporter != null) {

solr/core/src/java/org/apache/solr/metrics/reporters/solr/SolrClusterReporter.java

@@ -179,12 +179,17 @@ public class SolrClusterReporter extends SolrMetricReporter {
   }
 
   @Override
-  protected void validate() throws IllegalStateException {
+  protected void doInit() {
     if (reports.isEmpty()) { // set defaults
       reports = DEFAULT_REPORTS;
     }
   }
 
+  @Override
+  protected void validate() throws IllegalStateException {
+    // Nothing to validate
+  }
+
   @Override
   public void close() throws IOException {
     if (reporter != null) {

solr/core/src/java/org/apache/solr/metrics/reporters/solr/SolrShardReporter.java

@@ -113,11 +113,16 @@ public class SolrShardReporter extends SolrMetricReporter {
   }
 
   @Override
-  protected void validate() throws IllegalStateException {
+  protected void doInit() {
     if (filters.isEmpty()) {
       filters = DEFAULT_FILTERS;
     }
-    // start in inform(...) only when core is available
+    // start in setCore(SolrCore) when core is available
+  }
+
+  @Override
+  protected void validate() throws IllegalStateException {
+    // Nothing to validate
   }
 
   @Override

solr/core/src/test/org/apache/solr/metrics/reporters/MockMetricReporter.java

@@ -22,7 +22,6 @@ import java.util.NoSuchElementException;
 
 import com.codahale.metrics.Metric;
 import com.codahale.metrics.MetricRegistry;
-import org.apache.solr.core.PluginInfo;
 import org.apache.solr.metrics.SolrMetricManager;
 import org.apache.solr.metrics.SolrMetricReporter;
 
@@ -39,8 +38,7 @@ public class MockMetricReporter extends SolrMetricReporter {
   }
 
   @Override
-  public void init(PluginInfo pluginInfo) {
+  protected void doInit() {
-    super.init(pluginInfo);
     didInit = true;
   }