Avoid mock thread factory and simplify construction

5 files changed, 32 insertions, 50 deletions

diff --git a/container-core/src/main/java/com/yahoo/container/handler/VipStatus.java b/container-core/src/main/java/com/yahoo/container/handler/VipStatus.java
index e1b5b769906..876d05b7be9 100644
--- a/container-core/src/main/java/com/yahoo/container/handler/VipStatus.java
+++ b/container-core/src/main/java/com/yahoo/container/handler/VipStatus.java
@@ -50,7 +50,7 @@ public class VipStatus {
 
     /** For testing */
     public VipStatus(QrSearchersConfig dispatchers, ClustersStatus clustersStatus) {
-        this(dispatchers, new VipStatusConfig.Builder().build(), clustersStatus, new StateMonitor());
+        this(dispatchers, new VipStatusConfig.Builder().build(), clustersStatus, StateMonitor.createForTesting());
     }
 
     @Inject
diff --git a/container-core/src/main/java/com/yahoo/container/jdisc/state/StateMonitor.java b/container-core/src/main/java/com/yahoo/container/jdisc/state/StateMonitor.java
index 066118294a0..ccd0864b3ab 100644
--- a/container-core/src/main/java/com/yahoo/container/jdisc/state/StateMonitor.java
+++ b/container-core/src/main/java/com/yahoo/container/jdisc/state/StateMonitor.java
@@ -9,6 +9,7 @@ import com.yahoo.jdisc.application.MetricConsumer;
 import com.yahoo.jdisc.core.SystemTimer;
 
 import java.util.Map;
+import java.util.Optional;
 import java.util.TreeSet;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.Executors;
@@ -32,7 +33,7 @@ public class StateMonitor extends AbstractComponent {
     public enum Status {up, down, initializing}
 
     private final CopyOnWriteArrayList<StateMetricConsumer> consumers = new CopyOnWriteArrayList<>();
-    private final ScheduledExecutorService executor;
+    private final Optional<ScheduledExecutorService> executor;
     private final Timer timer;
     private final long snapshotIntervalMs;
     private volatile long lastSnapshotTimeMs;
@@ -40,41 +41,28 @@ public class StateMonitor extends AbstractComponent {
     private volatile Status status;
     private final TreeSet<String> valueNames = new TreeSet<>();
 
-    /** For testing */
-    public StateMonitor() {
-        this(new HealthMonitorConfig.Builder().build(), new SystemTimer());
-    }
-
     @Inject
     public StateMonitor(HealthMonitorConfig config, Timer timer) {
-        this(config,
-             timer,
-             runnable -> {
-                 Thread thread = new Thread(runnable, "StateMonitor");
-                 thread.setDaemon(true);
-                 return thread;
-             });
+        this(config, timer, runnable -> {
+            Thread thread = new Thread(runnable, "StateMonitor");
+            thread.setDaemon(true);
+            return thread;
+        });
     }
 
-    StateMonitor(HealthMonitorConfig config, Timer timer, ThreadFactory threadFactory) {
-        this((long)(config.snapshot_interval() * TimeUnit.SECONDS.toMillis(1)),
-             Status.valueOf(config.initialStatus()),
-             timer, threadFactory, true);
+    public static StateMonitor createForTesting() {
+        return new StateMonitor(new HealthMonitorConfig.Builder().build(), new SystemTimer());
     }
 
-    /* Public for testing only */
-    public StateMonitor(long snapshotIntervalMS, Status status, Timer timer, ThreadFactory threadFactory,
-                        boolean start) {
+    /** Non-private only for unit testing this class. */
+    StateMonitor(HealthMonitorConfig config, Timer timer, ThreadFactory threadFactory) {
         this.timer = timer;
-        this.snapshotIntervalMs = snapshotIntervalMS;
+        this.snapshotIntervalMs = (long)(config.snapshot_interval() * TimeUnit.SECONDS.toMillis(1));
         this.lastSnapshotTimeMs = timer.currentTimeMillis();
-        this.status = status;
-        this.executor = Executors.newSingleThreadScheduledExecutor(threadFactory);
-
-        if (start) {
-            executor.scheduleAtFixedRate(this::updateSnapshot, snapshotIntervalMS,
-                    snapshotIntervalMS, TimeUnit.MILLISECONDS);
-        }
+        this.status = Status.valueOf(config.initialStatus());
+        this.executor = Optional.ofNullable(threadFactory).map(Executors::newSingleThreadScheduledExecutor);
+        this.executor.ifPresent(exec -> exec.scheduleAtFixedRate(this::updateSnapshot, snapshotIntervalMs,
+                snapshotIntervalMs, TimeUnit.MILLISECONDS));
     }
 
     /** Returns a metric consumer for jDisc which will write metrics back to this */
@@ -136,13 +124,16 @@ public class StateMonitor extends AbstractComponent {
 
     @Override
     public void deconstruct() {
-        executor.shutdown();
-        try {
-            executor.awaitTermination(5, TimeUnit.SECONDS);
-        } catch (InterruptedException e) { }
+        executor.ifPresent(exec -> {
+            exec.shutdown();
+            try {
+                exec.awaitTermination(5, TimeUnit.SECONDS);
+            } catch (InterruptedException e) {
+            }
 
-        if (!executor.isTerminated()) {
-            log.warning("StateMonitor failed to terminate within 5 seconds of interrupt signal. Ignoring.");
-        }
+            if (!exec.isTerminated()) {
+                log.warning("StateMonitor failed to terminate within 5 seconds of interrupt signal. Ignoring.");
+            }
+        });
     }
 }
diff --git a/container-core/src/test/java/com/yahoo/container/handler/VipStatusTestCase.java b/container-core/src/test/java/com/yahoo/container/handler/VipStatusTestCase.java
index d26d7f7f013..3f33efa1993 100644
--- a/container-core/src/test/java/com/yahoo/container/handler/VipStatusTestCase.java
+++ b/container-core/src/test/java/com/yahoo/container/handler/VipStatusTestCase.java
@@ -4,7 +4,6 @@ package com.yahoo.container.handler;
 import com.yahoo.container.QrSearchersConfig;
 import com.yahoo.container.core.VipStatusConfig;
 import com.yahoo.container.jdisc.state.StateMonitor;
-import com.yahoo.jdisc.core.SystemTimer;
 import org.junit.Test;
 
 import static org.junit.Assert.assertEquals;
@@ -140,11 +139,9 @@ public class VipStatusTestCase {
     }
 
     private static StateMonitor createStateMonitor(StateMonitor.Status startState) {
-        return new StateMonitor(1000, startState, new SystemTimer(), runnable -> {
-            Thread thread = new Thread(runnable, "StateMonitor");
-            thread.setDaemon(true);
-            return thread;
-        }, true);
+        StateMonitor stateMonitor = StateMonitor.createForTesting();
+        stateMonitor.status(startState);
+        return stateMonitor;
     }
 
     private static void removeFromRotation(String[] clusters, VipStatus v) {
diff --git a/container-core/src/test/java/com/yahoo/container/jdisc/state/StateHandlerTestBase.java b/container-core/src/test/java/com/yahoo/container/jdisc/state/StateHandlerTestBase.java
index 6b44115016f..57a9ba4abdb 100644
--- a/container-core/src/test/java/com/yahoo/container/jdisc/state/StateHandlerTestBase.java
+++ b/container-core/src/test/java/com/yahoo/container/jdisc/state/StateHandlerTestBase.java
@@ -22,14 +22,12 @@ import org.junit.Before;
 import java.io.InputStreamReader;
 import java.io.Reader;
 import java.nio.charset.StandardCharsets;
-import java.util.concurrent.ThreadFactory;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicLong;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
-import static org.mockito.Mockito.mock;
 
 /**
  * @author Simon Thoresen Hult
@@ -59,8 +57,7 @@ public class StateHandlerTestBase {
                         new HealthMonitorConfig.Builder()
                                 .snapshot_interval(TimeUnit.MILLISECONDS.toSeconds(SNAPSHOT_INTERVAL))
                                 .initialStatus("up"));
-        ThreadFactory threadFactory = ignored -> mock(Thread.class);
-        this.monitor = new StateMonitor(healthMonitorConfig, timer, threadFactory);
+        this.monitor = new StateMonitor(healthMonitorConfig, timer, null);
         builder.guiceModules().install(new AbstractModule() {
 
             @Override
diff --git a/container-core/src/test/java/com/yahoo/container/jdisc/state/StateMonitorBenchmarkTest.java b/container-core/src/test/java/com/yahoo/container/jdisc/state/StateMonitorBenchmarkTest.java
index 3892f81b8b5..22a2fc274c7 100644
--- a/container-core/src/test/java/com/yahoo/container/jdisc/state/StateMonitorBenchmarkTest.java
+++ b/container-core/src/test/java/com/yahoo/container/jdisc/state/StateMonitorBenchmarkTest.java
@@ -2,12 +2,10 @@
 package com.yahoo.container.jdisc.state;
 
 import com.google.inject.Provider;
-import com.yahoo.container.jdisc.config.HealthMonitorConfig;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.jdisc.application.ContainerThread;
 import com.yahoo.jdisc.application.MetricConsumer;
 import com.yahoo.jdisc.application.MetricProvider;
-import com.yahoo.jdisc.core.SystemTimer;
 import org.junit.Test;
 
 import java.util.ArrayList;
@@ -32,8 +30,7 @@ public class StateMonitorBenchmarkTest {
 
     @Test
     public void requireThatHealthMonitorDoesNotBlockMetricThreads() throws Exception {
-        StateMonitor monitor = new StateMonitor(new HealthMonitorConfig(new HealthMonitorConfig.Builder()),
-                                                new SystemTimer());
+        StateMonitor monitor = StateMonitor.createForTesting();
         Provider<MetricConsumer> provider = MetricConsumerProviders.wrap(monitor);
         performUpdates(provider, 8);
         for (int i = 1; i <= NUM_THREADS; i *= 2) {