Merge pull request #10883 from vespa-engine/balder/shutdown-monitoring

- Shut down monitoring thread.

15 files changed, 81 insertions, 47 deletions

diff --git a/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterSearcher.java b/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterSearcher.java
index 1f621eb926c..0c8e564578b 100644
--- a/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterSearcher.java
+++ b/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterSearcher.java
@@ -7,7 +7,6 @@ import com.yahoo.component.chain.dependencies.After;
 import com.yahoo.container.QrSearchersConfig;
 import com.yahoo.container.handler.VipStatus;
 import com.yahoo.jdisc.Metric;
-import com.yahoo.net.HostName;
 import com.yahoo.prelude.IndexFacts;
 import com.yahoo.prelude.fastsearch.ClusterParams;
 import com.yahoo.prelude.fastsearch.DocumentdbInfoConfig;
@@ -27,8 +26,6 @@ import com.yahoo.vespa.config.search.DispatchConfig;
 import com.yahoo.vespa.streamingvisitors.VdsStreamingSearcher;
 import org.apache.commons.lang.StringUtils;
 
-import java.net.InetAddress;
-import java.net.UnknownHostException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -371,6 +368,10 @@ public class ClusterSearcher extends Searcher {
     }
 
     @Override
-    public void deconstruct() { }
+    public void deconstruct() {
+        if (server != null) {
+            server.shutDown();
+        }
+    }
 
 }
diff --git a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java
index b0b3a7800e9..9a4913b3840 100644
--- a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java
+++ b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java
@@ -173,4 +173,9 @@ public class FastSearcher extends VespaBackEndSearcher {
         return getLogger().isLoggable(Level.FINE);
     }
 
+    @Override
+    public void shutDown() {
+        super.shutDown();
+        dispatcher.shutDown();
+    }
 }
diff --git a/container-search/src/main/java/com/yahoo/prelude/fastsearch/VespaBackEndSearcher.java b/container-search/src/main/java/com/yahoo/prelude/fastsearch/VespaBackEndSearcher.java
index 8f4b49ac71e..bc3ac6cdef1 100644
--- a/container-search/src/main/java/com/yahoo/prelude/fastsearch/VespaBackEndSearcher.java
+++ b/container-search/src/main/java/com/yahoo/prelude/fastsearch/VespaBackEndSearcher.java
@@ -392,4 +392,6 @@ public abstract class VespaBackEndSearcher extends PingableSearcher {
         return getLogger().isLoggable(Level.FINE);
     }
 
+    public void shutDown() { }
+
 }
diff --git a/container-search/src/main/java/com/yahoo/search/cluster/ClusterMonitor.java b/container-search/src/main/java/com/yahoo/search/cluster/ClusterMonitor.java
index 22c7f59872c..a016f7d695c 100644
--- a/container-search/src/main/java/com/yahoo/search/cluster/ClusterMonitor.java
+++ b/container-search/src/main/java/com/yahoo/search/cluster/ClusterMonitor.java
@@ -9,7 +9,9 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -26,9 +28,9 @@ public class ClusterMonitor<T> {
 
     private static Logger log = Logger.getLogger(ClusterMonitor.class.getName());
 
-    private NodeManager<T> nodeManager;
+    private final NodeManager<T> nodeManager;
 
-    private MonitorThread monitorThread;
+    private final MonitorThread monitorThread;
 
     private volatile boolean shutdown = false;
 
@@ -119,28 +121,35 @@ public class ClusterMonitor<T> {
         }
 
         public void run() {
-            log.fine("Starting cluster monitor thread");
+            log.info("Starting cluster monitor thread " + getName());
             // Pings must happen in a separate thread from this to handle timeouts
             // By using a cached thread pool we ensured that 1) a single thread will be used
             // for all pings when there are no problems (important because it ensures that
             // any thread local connections are reused) 2) a new thread will be started to execute
             // new pings when a ping is not responding
-            Executor pingExecutor=Executors.newCachedThreadPool(ThreadFactoryFactory.getDaemonThreadFactory("search.ping"));
+            ExecutorService pingExecutor=Executors.newCachedThreadPool(ThreadFactoryFactory.getDaemonThreadFactory("search.ping"));
             while (!isInterrupted()) {
                 try {
                     Thread.sleep(configuration.getCheckInterval());
                     log.finest("Activating ping");
                     ping(pingExecutor);
                 }
-                catch (Exception e) {
+                catch (Throwable e) {
                     if (shutdown && e instanceof InterruptedException) {
                         break;
+                    } else if ( ! (e instanceof Exception) ) {
+                        log.log(Level.WARNING,"Error in monitor thread, will quit", e);
+                        break;
                     } else {
-                        log.log(Level.WARNING,"Error in monitor thread",e);
+                        log.log(Level.WARNING,"Exception in monitor thread", e);
                     }
                 }
             }
-            log.fine("Stopped cluster monitor thread");
+            pingExecutor.shutdown();
+            try {
+                pingExecutor.awaitTermination(10, TimeUnit.SECONDS);
+            } catch (InterruptedException e) { }
+            log.info("Stopped cluster monitor thread " + getName());
         }
 
     }
diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java b/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java
index 7369b33e82d..ddd319b7bcb 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java
@@ -195,6 +195,10 @@ public class Dispatcher extends AbstractComponent {
         return Optional.empty();
     }
 
+    public void shutDown() {
+        searchCluster.shutDown();
+    }
+
     private void emitDispatchMetric(Optional<SearchInvoker> invoker) {
         if (invoker.isEmpty()) {
             metric.add(FDISPATCH_METRIC, 1, metricContext);
diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Node.java b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Node.java
index b47f2fefa5b..53ecd59d991 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Node.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Node.java
@@ -16,17 +16,15 @@ public class Node {
     private final int key;
     private int pathIndex;
     private final String hostname;
-    private final int fs4port;
-    final int group;
+    private final int group;
 
     private final AtomicBoolean statusIsKnown = new AtomicBoolean(false);
     private final AtomicBoolean working = new AtomicBoolean(true);
     private final AtomicLong activeDocuments = new AtomicLong(0);
 
-    public Node(int key, String hostname, int fs4port, int group) {
+    public Node(int key, String hostname, int group) {
         this.key = key;
         this.hostname = hostname;
-        this.fs4port = fs4port;
         this.group = group;
     }
 
@@ -41,14 +39,15 @@ public class Node {
 
     public String hostname() { return hostname; }
 
-    public int fs4port() { return fs4port; }
-
     /** Returns the id of this group this node belongs to */
     public int group() { return group; }
 
     public void setWorking(boolean working) {
         this.statusIsKnown.lazySet(true);
         this.working.lazySet(working);
+        if ( ! working ) {
+            activeDocuments.set(0);
+        }
     }
 
     /** Returns whether this node is currently responding to requests, or null if status is not known */
@@ -57,17 +56,17 @@ public class Node {
     }
 
     /** Updates the active documents on this node */
-    public void setActiveDocuments(long activeDocuments) {
+    void setActiveDocuments(long activeDocuments) {
         this.activeDocuments.set(activeDocuments);
     }
 
     /** Returns the active documents on this node. If unknown, 0 is returned. */
-    public long getActiveDocuments() {
-        return this.activeDocuments.get();
+    long getActiveDocuments() {
+        return activeDocuments.get();
     }
 
     @Override
-    public int hashCode() { return Objects.hash(hostname, fs4port); }
+    public int hashCode() { return Objects.hash(hostname, key, pathIndex, group); }
 
     @Override
     public boolean equals(Object o) {
@@ -75,11 +74,15 @@ public class Node {
         if ( ! (o instanceof Node)) return false;
         Node other = (Node)o;
         if ( ! Objects.equals(this.hostname, other.hostname)) return false;
-        if ( ! Objects.equals(this.fs4port, other.fs4port)) return false;
+        if ( ! Objects.equals(this.key, other.key)) return false;
+        if ( ! Objects.equals(this.pathIndex, other.pathIndex)) return false;
+        if ( ! Objects.equals(this.group, other.group)) return false;
+
         return true;
     }
 
     @Override
-    public String toString() { return "search node " + hostname + ":" + fs4port + " in group " + group; }
+    public String toString() { return "search node key = " + key + " hostname = "+ hostname + " path = " + pathIndex + " in group " + group +
+            " statusIsKnown = " + statusIsKnown.get() + " working" + working.get() + " activeDocs = " + activeDocuments.get(); }
 
 }
diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java
index a55a970e8ff..7380140ac4a 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java
@@ -73,7 +73,7 @@ public class SearchCluster implements NodeManager<Node> {
         }
         this.groups = groupsBuilder.build();
         LinkedHashMap<Integer, Group> groupIntroductionOrder = new LinkedHashMap<>();
-        nodes.forEach(node -> groupIntroductionOrder.put(node.group(), groups.get(node.group)));
+        nodes.forEach(node -> groupIntroductionOrder.put(node.group(), groups.get(node.group())));
         this.orderedGroups = ImmutableList.<Group>builder().addAll(groupIntroductionOrder.values()).build();
 
         // Index nodes by host
@@ -91,6 +91,10 @@ public class SearchCluster implements NodeManager<Node> {
         this.clusterMonitor = new ClusterMonitor<>(this);
     }
 
+    public void shutDown() {
+        clusterMonitor.shutdown();
+    }
+
     public void startClusterMonitoring(PingFactory pingFactory) {
         this.pingFactory = pingFactory;
 
@@ -141,7 +145,7 @@ public class SearchCluster implements NodeManager<Node> {
         }
         for (DispatchConfig.Node node : dispatchConfig.node()) {
             if (filter.test(node)) {
-                nodesBuilder.add(new Node(node.key(), node.host(), node.fs4port(), node.group()));
+                nodesBuilder.add(new Node(node.key(), node.host(), node.group()));
             }
         }
         return nodesBuilder.build();
@@ -409,14 +413,20 @@ public class SearchCluster implements NodeManager<Node> {
 
     private void trackGroupCoverageChanges(int index, Group group, boolean fullCoverage, long averageDocuments) {
         boolean changed = group.isFullCoverageStatusChanged(fullCoverage);
-        if (changed) {
+        if (changed || !fullCoverage) {
             int requiredNodes = groupSize() - dispatchConfig.maxNodesDownPerGroup();
             if (fullCoverage) {
                 log.info(() -> String.format("Group %d is now good again (%d/%d active docs, coverage %d/%d)",
                                              index, group.getActiveDocuments(), averageDocuments, group.workingNodes(), groupSize()));
             } else {
-                log.warning(() -> String.format("Coverage of group %d is only %d/%d (requires %d)",
-                                                index, group.workingNodes(), groupSize(), requiredNodes));
+                StringBuilder missing = new StringBuilder();
+                for (var node : group.nodes()) {
+                    if (node.isWorking() != Boolean.TRUE) {
+                        missing.append('\n').append(node.toString());
+                    }
+                }
+                log.warning(() -> String.format("Coverage of group %d is only %d/%d (requires %d) (%d/%d active docs)\nFailed nodes are:\n%s",
+                        index, group.workingNodes(), groupSize(), requiredNodes, group.getActiveDocuments(), averageDocuments, missing.toString()));
             }
         }
     }
diff --git a/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTestCase.java b/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTestCase.java
index eb4d65693bb..4011611b049 100644
--- a/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTestCase.java
+++ b/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTestCase.java
@@ -81,7 +81,7 @@ public class FastSearcherTestCase {
     @Test
     public void testSinglePassGroupingIsForcedWithSingleNodeGroups() {
         FastSearcher fastSearcher = new FastSearcher("container.0",
-                                                     MockDispatcher.create(Collections.singletonList(new Node(0, "host0", 123, 0))),
+                                                     MockDispatcher.create(Collections.singletonList(new Node(0, "host0", 0))),
                                                      new SummaryParameters(null),
                                                      new ClusterParams("testhittype"),
                                                      documentdbInfoConfig);
@@ -102,7 +102,7 @@ public class FastSearcherTestCase {
 
     @Test
     public void testSinglePassGroupingIsNotForcedWithSingleNodeGroups() {
-        MockDispatcher dispatcher = MockDispatcher.create(ImmutableList.of(new Node(0, "host0", 123, 0), new Node(2, "host1", 123, 0)));
+        MockDispatcher dispatcher = MockDispatcher.create(ImmutableList.of(new Node(0, "host0", 0), new Node(2, "host1", 0)));
 
         FastSearcher fastSearcher = new FastSearcher("container.0",
                                                      dispatcher,
diff --git a/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/MockDispatcher.java b/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/MockDispatcher.java
index 0aa91442712..4fbbd9dd936 100644
--- a/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/MockDispatcher.java
+++ b/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/MockDispatcher.java
@@ -39,7 +39,6 @@ class MockDispatcher extends Dispatcher {
         for (Node node : nodes) {
             DispatchConfig.Node.Builder dispatchConfigNodeBuilder = new DispatchConfig.Node.Builder();
             dispatchConfigNodeBuilder.host(node.hostname());
-            dispatchConfigNodeBuilder.fs4port(node.fs4port());
             dispatchConfigNodeBuilder.port(0); // Mandatory, but currently not used here
             dispatchConfigNodeBuilder.group(node.group());
             dispatchConfigNodeBuilder.key(key++); // not used
diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java
index 310f536f961..3d544f5c114 100644
--- a/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java
+++ b/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java
@@ -70,7 +70,7 @@ public class DispatcherTest {
         SearchCluster cl = new MockSearchCluster("1", 0, 0) {
             @Override
             public Optional<Node> localCorpusDispatchTarget() {
-                return Optional.of(new Node(1, "test", 123, 1));
+                return Optional.of(new Node(1, "test", 1));
             }
         };
         MockInvokerFactory invokerFactory = new MockInvokerFactory(cl, (n, a) -> true);
diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java
index 1ebf7940f25..0496194f8ed 100644
--- a/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java
+++ b/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java
@@ -28,7 +28,7 @@ import static org.junit.Assert.assertThat;
 public class LoadBalancerTest {
     @Test
     public void requireThatLoadBalancerServesSingleNodeSetups() {
-        Node n1 = new Node(0, "test-node1", 0, 0);
+        Node n1 = new Node(0, "test-node1", 0);
         SearchCluster cluster = new SearchCluster("a", createDispatchConfig(n1), 1, null);
         LoadBalancer lb = new LoadBalancer(cluster, true);
 
@@ -41,8 +41,8 @@ public class LoadBalancerTest {
 
     @Test
     public void requireThatLoadBalancerServesMultiGroupSetups() {
-        Node n1 = new Node(0, "test-node1", 0, 0);
-        Node n2 = new Node(1, "test-node2", 1, 1);
+        Node n1 = new Node(0, "test-node1", 0);
+        Node n2 = new Node(1, "test-node2", 1);
         SearchCluster cluster = new SearchCluster("a", createDispatchConfig(n1, n2), 1, null);
         LoadBalancer lb = new LoadBalancer(cluster, true);
 
@@ -55,10 +55,10 @@ public class LoadBalancerTest {
 
     @Test
     public void requireThatLoadBalancerServesClusteredGroups() {
-        Node n1 = new Node(0, "test-node1", 0, 0);
-        Node n2 = new Node(1, "test-node2", 1, 0);
-        Node n3 = new Node(0, "test-node3", 0, 1);
-        Node n4 = new Node(1, "test-node4", 1, 1);
+        Node n1 = new Node(0, "test-node1", 0);
+        Node n2 = new Node(1, "test-node2", 0);
+        Node n3 = new Node(0, "test-node3", 1);
+        Node n4 = new Node(1, "test-node4", 1);
         SearchCluster cluster = new SearchCluster("a", createDispatchConfig(n1, n2, n3, n4), 2, null);
         LoadBalancer lb = new LoadBalancer(cluster, true);
 
@@ -68,8 +68,8 @@ public class LoadBalancerTest {
 
     @Test
     public void requireThatLoadBalancerReturnsDifferentGroups() {
-        Node n1 = new Node(0, "test-node1", 0, 0);
-        Node n2 = new Node(1, "test-node2", 1, 1);
+        Node n1 = new Node(0, "test-node1", 0);
+        Node n2 = new Node(1, "test-node2", 1);
         SearchCluster cluster = new SearchCluster("a", createDispatchConfig(n1, n2), 1, null);
         LoadBalancer lb = new LoadBalancer(cluster, true);
 
diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/MockInvoker.java b/container-search/src/test/java/com/yahoo/search/dispatch/MockInvoker.java
index 2fe434d6f3f..c5fbda7c2f5 100644
--- a/container-search/src/test/java/com/yahoo/search/dispatch/MockInvoker.java
+++ b/container-search/src/test/java/com/yahoo/search/dispatch/MockInvoker.java
@@ -19,7 +19,7 @@ class MockInvoker extends SearchInvoker {
     private List<Hit> hits;
 
     protected MockInvoker(int key, Coverage coverage) {
-        super(Optional.of(new Node(key, "?", 0, 0)));
+        super(Optional.of(new Node(key, "?", 0)));
         this.coverage = coverage;
     }
 
diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/MockSearchCluster.java b/container-search/src/test/java/com/yahoo/search/dispatch/MockSearchCluster.java
index e3ff54102d4..0bcc30d9b10 100644
--- a/container-search/src/test/java/com/yahoo/search/dispatch/MockSearchCluster.java
+++ b/container-search/src/test/java/com/yahoo/search/dispatch/MockSearchCluster.java
@@ -38,7 +38,7 @@ public class MockSearchCluster extends SearchCluster {
         for (int group = 0; group < groups; group++) {
             List<Node> nodes = new ArrayList<>();
             for (int node = 0; node < nodesPerGroup; node++) {
-                Node n = new Node(dk, "host" + dk, -1, group);
+                Node n = new Node(dk, "host" + dk, group);
                 n.setWorking(true);
                 nodes.add(n);
                 hostBuilder.put(n.hostname(), n);
@@ -124,8 +124,9 @@ public class MockSearchCluster extends SearchCluster {
             builder.minWaitAfterCoverageFactor(0);
             builder.maxWaitAfterCoverageFactor(0.5);
         }
+        int port = 10000;
         for (Node n : nodes) {
-            builder.node(new DispatchConfig.Node.Builder().key(n.key()).host(n.hostname()).port(n.fs4port()).group(n.group()));
+            builder.node(new DispatchConfig.Node.Builder().key(n.key()).host(n.hostname()).port(port++).group(n.group()));
         }
         return new DispatchConfig(builder);
     }
diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/rpc/RpcSearchInvokerTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/rpc/RpcSearchInvokerTest.java
index d629bd36bb1..c07bf119782 100644
--- a/container-search/src/test/java/com/yahoo/search/dispatch/rpc/RpcSearchInvokerTest.java
+++ b/container-search/src/test/java/com/yahoo/search/dispatch/rpc/RpcSearchInvokerTest.java
@@ -36,7 +36,7 @@ public class RpcSearchInvokerTest {
         var mockClient = parameterCollectorClient(compressionTypeHolder, payloadHolder, lengthHolder);
         var mockPool = new RpcResourcePool(ImmutableMap.of(7, mockClient.createConnection("foo", 123)));
         @SuppressWarnings("resource")
-        var invoker = new RpcSearchInvoker(mockSearcher(), new Node(7, "seven", 77, 1), mockPool);
+        var invoker = new RpcSearchInvoker(mockSearcher(), new Node(7, "seven", 1), mockPool);
 
         Query q = new Query("search/?query=test&hits=10&offset=3");
         invoker.sendSearchRequest(q);
diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/searchcluster/SearchClusterTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/searchcluster/SearchClusterTest.java
index f29d6ddf324..f42185e955f 100644
--- a/container-search/src/test/java/com/yahoo/search/dispatch/searchcluster/SearchClusterTest.java
+++ b/container-search/src/test/java/com/yahoo/search/dispatch/searchcluster/SearchClusterTest.java
@@ -63,7 +63,7 @@ public class SearchClusterTest {
             for (String name : nodeNames) {
                 int key = nodes.size() % nodesPergroup;
                 int group = nodes.size() / nodesPergroup;
-                nodes.add(new Node(key, name, 13333, group));
+                nodes.add(new Node(key, name, group));
                 numDocsPerNode.add(new AtomicInteger(1));
                 pingCounts.add(new AtomicInteger(0));
             }
@@ -132,7 +132,7 @@ public class SearchClusterTest {
 
             @Override
             public Callable<Pong> createPinger(Node node, ClusterMonitor<Node> monitor) {
-                int index = node.group*numPerGroup + node.key();
+                int index = node.group() * numPerGroup + node.key();
                 return new Pinger(activeDocs.get(index), pingCounts.get(index));
             }
         }