Do not run ZKMetricsUpdater when testing

Also rewrite to use executors and make sure to shutdown properly

3 files changed, 50 insertions, 23 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/monitoring/Metrics.java b/configserver/src/main/java/com/yahoo/vespa/config/server/monitoring/Metrics.java
index 87e9fa287a4..249c8639ea9 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/monitoring/Metrics.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/monitoring/Metrics.java
@@ -3,6 +3,8 @@ package com.yahoo.vespa.config.server.monitoring;
 
 import com.google.inject.Inject;
 import com.yahoo.cloud.config.ZookeeperServerConfig;
+import com.yahoo.component.AbstractComponent;
+import com.yahoo.concurrent.DaemonThreadFactory;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.container.jdisc.config.HealthMonitorConfig;
@@ -14,14 +16,17 @@ import com.yahoo.statistics.Counter;
 
 import java.util.*;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
 
 /**
- * Statistics for server. The statistics framework takes care of logging.
+ * Metrics for config server. The statistics framework takes care of logging.
  *
  * @author Harald Musum
- * @since 4.2
  */
-public class Metrics extends TimerTask implements MetricUpdaterFactory {
+public class Metrics extends AbstractComponent implements MetricUpdaterFactory, Runnable {
 
     private static final java.util.logging.Logger log = java.util.logging.Logger.getLogger(Metrics.class.getName());
     private static final String METRIC_REQUESTS = getMetricName("requests");
@@ -33,23 +38,34 @@ public class Metrics extends TimerTask implements MetricUpdaterFactory {
     private final Counter failedRequests;
     private final Counter procTimeCounter;
     private final Metric metric;
-    private final ZKMetricUpdater zkMetricUpdater;
+    private final Optional<ZKMetricUpdater> zkMetricUpdater;
 
     // TODO The map is the key for now
     private final Map<Map<String, String>, MetricUpdater> metricUpdaters = new ConcurrentHashMap<>();
-    private final Timer timer = new Timer();
+    private final Optional<ScheduledExecutorService> executorService;
 
     @Inject
     public Metrics(Metric metric, Statistics statistics, HealthMonitorConfig healthMonitorConfig, ZookeeperServerConfig zkServerConfig) {
+        this(metric, statistics, healthMonitorConfig, zkServerConfig, true);
+    }
+
+    private Metrics(Metric metric, Statistics statistics, HealthMonitorConfig healthMonitorConfig,
+                    ZookeeperServerConfig zkServerConfig, boolean createZkMetricUpdater) {
         this.metric = metric;
         requests = createCounter(METRIC_REQUESTS, statistics);
         failedRequests = createCounter(METRIC_FAILED_REQUESTS, statistics);
         procTimeCounter = createCounter("procTime", statistics);
 
-        log.log(LogLevel.DEBUG, "Metric update interval is " + healthMonitorConfig.snapshot_interval() + " seconds");
-        long intervalMs = (long) (healthMonitorConfig.snapshot_interval() * 1000);
-        timer.scheduleAtFixedRate(this, 20000, intervalMs);
-        zkMetricUpdater = new ZKMetricUpdater(zkServerConfig, 19500, intervalMs);
+        if (createZkMetricUpdater) {
+            log.log(LogLevel.DEBUG, "Metric update interval is " + healthMonitorConfig.snapshot_interval() + " seconds");
+            long intervalMs = (long) (healthMonitorConfig.snapshot_interval() * 1000);
+            executorService = Optional.of(new ScheduledThreadPoolExecutor(1, new DaemonThreadFactory("configserver-metrics")));
+            executorService.get().scheduleAtFixedRate(this, 20000, intervalMs, TimeUnit.MILLISECONDS);
+            zkMetricUpdater = Optional.of(new ZKMetricUpdater(zkServerConfig, 19500, intervalMs));
+        } else {
+            executorService = Optional.empty();
+            zkMetricUpdater = Optional.empty();
+        }
     }
 
     public static Metrics createTestMetrics() {
@@ -58,14 +74,13 @@ public class Metrics extends TimerTask implements MetricUpdaterFactory {
         HealthMonitorConfig.Builder builder = new HealthMonitorConfig.Builder();
         builder.snapshot_interval(60.0);
         ZookeeperServerConfig.Builder zkBuilder = new ZookeeperServerConfig.Builder().myid(1);
-        return new Metrics(metric, statistics, new HealthMonitorConfig(builder), new ZookeeperServerConfig(zkBuilder));
+        return new Metrics(metric, statistics, new HealthMonitorConfig(builder), new ZookeeperServerConfig(zkBuilder), false);
     }
 
     private Counter createCounter(String name, Statistics statistics) {
         return new Counter(name, statistics, false);
     }
 
-
     void incrementRequests(Metric.Context metricContext) {
         requests.increment(1);
         metric.add(METRIC_REQUESTS, 1, metricContext);
@@ -126,8 +141,12 @@ public class Metrics extends TimerTask implements MetricUpdaterFactory {
             }
         }
         setRegularMetrics();
-        zkMetricUpdater.getZKMetrics().forEach((attr, val) -> metric.set(attr, val, null));
-        timer.purge();
+        zkMetricUpdater.ifPresent(updater -> updater.getZKMetrics().forEach((attr, val) -> metric.set(attr, val, null)));
+    }
+
+    public void deconstruct() {
+        executorService.ifPresent(ExecutorService::shutdown);
+        zkMetricUpdater.ifPresent(ZKMetricUpdater::shutdown);
     }
 
     private void setRegularMetrics() {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/monitoring/ZKMetricUpdater.java b/configserver/src/main/java/com/yahoo/vespa/config/server/monitoring/ZKMetricUpdater.java
index b2813be5456..62da6fcffbe 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/monitoring/ZKMetricUpdater.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/monitoring/ZKMetricUpdater.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.config.server.monitoring;
 
 import com.yahoo.cloud.config.ZookeeperServerConfig;
+import com.yahoo.concurrent.DaemonThreadFactory;
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
@@ -17,6 +18,8 @@ import java.util.Timer;
 import java.util.TimerTask;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicReference;
@@ -26,7 +29,7 @@ import java.util.regex.Pattern;
 
 import static com.yahoo.vespa.config.server.monitoring.Metrics.getMetricName;
 
-public class ZKMetricUpdater extends TimerTask {
+public class ZKMetricUpdater implements Runnable {
     private static final Logger log = Logger.getLogger(ZKMetricUpdater.class.getName());
 
     public static final String METRIC_ZK_ZNODES = getMetricName("zkZNodes");
@@ -35,19 +38,19 @@ public class ZKMetricUpdater extends TimerTask {
     public static final String METRIC_ZK_CONNECTIONS = getMetricName("zkConnections");
     public static final String METRIC_ZK_OUTSTANDING_REQUESTS = getMetricName("zkOutstandingRequests");
 
-    private final int CONNECTION_TIMEOUT_MS = 500;
-    private final int WRITE_TIMEOUT_MS = 250;
-    private final int READ_TIMEOUT_MS = 500;
+    private static final int CONNECTION_TIMEOUT_MS = 500;
+    private static final int WRITE_TIMEOUT_MS = 250;
+    private static final int READ_TIMEOUT_MS = 500;
 
     private AtomicReference<Map<String, Long>> zkMetrics = new AtomicReference<>(new HashMap<>());
-    private final Timer timer = new Timer();
+    private final ScheduledExecutorService executorService;
     private final int zkPort;
 
     public ZKMetricUpdater(ZookeeperServerConfig zkServerConfig, long delayMS, long intervalMS) {
         this.zkPort = zkServerConfig.clientPort();
-        if (intervalMS > 0) {
-            timer.scheduleAtFixedRate(this, delayMS, intervalMS);
-        }
+        if (intervalMS <= 0 ) throw new IllegalArgumentException("interval must be positive, was " + intervalMS + " ms");
+        this.executorService = new ScheduledThreadPoolExecutor(1, new DaemonThreadFactory("zkmetricupdater"));
+        this.executorService.scheduleAtFixedRate(this, delayMS, intervalMS, TimeUnit.MILLISECONDS);
     }
 
     private void setMetricAttribute(String attribute, long value, Map<String, Long> data) {
@@ -74,7 +77,10 @@ public class ZKMetricUpdater extends TimerTask {
     public void run() {
         Optional<String> report = retrieveReport();
         report.ifPresent(this::parseReport);
-        timer.purge();
+    }
+
+    public void shutdown() {
+        executorService.shutdown();
     }
 
     private Optional<String> retrieveReport() {
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/monitoring/ZKMetricUpdaterTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/monitoring/ZKMetricUpdaterTest.java
index ed089109759..607a2dca6c6 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/monitoring/ZKMetricUpdaterTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/monitoring/ZKMetricUpdaterTest.java
@@ -57,12 +57,14 @@ public class ZKMetricUpdaterTest {
         assertThat(reportedMetrics.get(ZKMetricUpdater.METRIC_ZK_LATENCY_MAX), equalTo(1234L));
         assertThat(reportedMetrics.get(ZKMetricUpdater.METRIC_ZK_OUTSTANDING_REQUESTS), equalTo(12L));
         assertThat(reportedMetrics.get(ZKMetricUpdater.METRIC_ZK_ZNODES), equalTo(4L));
+
+        updater.shutdown();
     }
 
     private ZKMetricUpdater buildUpdater() {
         ZookeeperServerConfig zkServerConfig = new ZookeeperServerConfig(
                 new ZookeeperServerConfig.Builder().clientPort(serverPort).myid(12345));
-        return new ZKMetricUpdater(zkServerConfig, 0, -1);
+        return new ZKMetricUpdater(zkServerConfig, 0, 100000);
     }
 
     private void setupTcpServer(Supplier<String> reportProvider) throws IOException {