Revert "Bratseth/vip logic"

10 files changed, 73 insertions, 114 deletions

diff --git a/container-search/src/main/java/com/yahoo/search/cluster/BaseNodeMonitor.java b/container-search/src/main/java/com/yahoo/search/cluster/BaseNodeMonitor.java
index d21ef35bcc2..59674d25402 100644
--- a/container-search/src/main/java/com/yahoo/search/cluster/BaseNodeMonitor.java
+++ b/container-search/src/main/java/com/yahoo/search/cluster/BaseNodeMonitor.java
@@ -26,25 +26,25 @@ public abstract class BaseNodeMonitor<T> {
     /** The object representing the monitored node */
     protected T node;
 
-    protected boolean isWorking = true;
+    protected boolean isWorking=true;
 
     /** Whether this node is quarantined for unstability */
-    protected boolean isQuarantined = false;
+    protected boolean isQuarantined=false;
 
     /** The last time this node failed, in ms */
-    protected long failedAt = 0;
+    protected long failedAt=0;
 
     /** The last time this node responded (failed or succeeded), in ms */
-    protected long respondedAt = 0;
+    protected long respondedAt=0;
 
     /** The last time this node responded successfully */
-    protected long succeededAt = 0;
+    protected long succeededAt=0;
 
     /** The configuration of this monitor */
     protected MonitorConfiguration configuration;
 
     /** Is the node we monitor part of an internal Vespa cluster or not */
-    private boolean internal;
+    private boolean internal=false;
 
     public BaseNodeMonitor(boolean internal) {
         this.internal=internal;
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 b79f6f49c19..ac0c8375f04 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
@@ -24,7 +24,7 @@ public class ClusterMonitor<T> {
 
     private MonitorConfiguration configuration = new MonitorConfiguration();
 
-    private static Logger log = Logger.getLogger(ClusterMonitor.class.getName());
+    private static Logger log=Logger.getLogger(ClusterMonitor.class.getName());
 
     private NodeManager<T> nodeManager;
 
@@ -69,7 +69,6 @@ public class ClusterMonitor<T> {
 
     /** Called from ClusterSearcher/NodeManager when a node failed */
     public synchronized void failed(T node, ErrorMessage error) {
-        nodeManager.statusIsKnown(node);
         BaseNodeMonitor<T> monitor = nodeMonitors.get(node);
         boolean wasWorking = monitor.isWorking();
         monitor.failed(error);
@@ -80,7 +79,6 @@ public class ClusterMonitor<T> {
 
     /** Called when a node responded */
     public synchronized void responded(T node) {
-        nodeManager.statusIsKnown(node);
         BaseNodeMonitor<T> monitor = nodeMonitors.get(node);
         boolean wasFailing =! monitor.isWorking();
         monitor.responded();
diff --git a/container-search/src/main/java/com/yahoo/search/cluster/NodeManager.java b/container-search/src/main/java/com/yahoo/search/cluster/NodeManager.java
index ef10680a4ae..9b20139e3c5 100644
--- a/container-search/src/main/java/com/yahoo/search/cluster/NodeManager.java
+++ b/container-search/src/main/java/com/yahoo/search/cluster/NodeManager.java
@@ -11,9 +11,6 @@ import java.util.concurrent.Executor;
  */
 public interface NodeManager<T> {
 
-    /** Called when we gain evidence about whether or not a node is working */
-    default void statusIsKnown(T node) { }
-
     /** Called when a failed node is working (ready for production) again */
     void working(T node);
 
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 af34fc3e106..58f73ea52cc 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
@@ -139,7 +139,7 @@ public class Dispatcher extends AbstractComponent {
         return invoker;
     }
 
-    /** Builds an invoker based on searchpath */
+    // build invoker based on searchpath
     private Optional<SearchInvoker> getSearchPathInvoker(Query query, VespaBackEndSearcher searcher) {
         String searchPath = query.getModel().getSearchPath();
         if (searchPath == null) return Optional.empty();
@@ -156,7 +156,7 @@ public class Dispatcher extends AbstractComponent {
     }
 
     private Optional<SearchInvoker> getInternalInvoker(Query query, VespaBackEndSearcher searcher) {
-        Optional<Node> directNode = searchCluster.localCorpusDispatchTarget();
+        Optional<Node> directNode = searchCluster.directDispatchTarget();
         if (directNode.isPresent()) {
             Node node = directNode.get();
             query.trace(false, 2, "Dispatching directly to ", node);
@@ -202,5 +202,4 @@ public class Dispatcher extends AbstractComponent {
             metric.add(INTERNAL_METRIC, 1, metricContext);
         }
     }
-
 }
diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Group.java b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Group.java
index 93cbd77cef7..bafc72b9b43 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Group.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Group.java
@@ -77,11 +77,6 @@ public class Group {
         return this.activeDocuments.get();
     }
 
-    public boolean isFullCoverageStatusChanged(boolean hasFullCoverageNow) {
-        boolean previousState = hasFullCoverage.getAndSet(hasFullCoverageNow);
-        return previousState != hasFullCoverageNow;
-    }
-
     @Override
     public String toString() { return "search group " + id; }
 
@@ -95,4 +90,8 @@ public class Group {
         return ((Group) other).id == this.id;
     }
 
+    public boolean isFullCoverageStatusChanged(boolean hasFullCoverageNow) {
+        boolean previousState = hasFullCoverage.getAndSet(hasFullCoverageNow);
+        return previousState != hasFullCoverageNow;
+    }
 }
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 0dd933fc24e..a71ce0354f9 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
@@ -19,7 +19,6 @@ public class Node {
     private final int fs4port;
     final int group;
 
-    private final AtomicBoolean statusIsKnown = new AtomicBoolean(false);
     private final AtomicBoolean working = new AtomicBoolean(true);
     private final AtomicLong activeDocuments = new AtomicLong(0);
 
@@ -46,13 +45,9 @@ public class Node {
     /** Returns the id of this group this node belongs to */
     public int group() { return group; }
 
-    /** Note that we know the status of this node */
-    public void setStatusIsKnown() { statusIsKnown.lazySet(true); }
-
-    /** Returns whether we know the status of this node */
-    public boolean getStatusIsKnown() { return statusIsKnown.get(); }
-
-    public void setWorking(boolean working) { this.working.lazySet(working); }
+    public void setWorking(boolean working) {
+        this.working.lazySet(working);
+    }
 
     /** Returns whether this node is currently responding to requests */
     public boolean isWorking() { return working.get(); }
@@ -82,5 +77,4 @@ public class Node {
 
     @Override
     public String toString() { return "search node " + hostname + ":" + fs4port + " in group " + group; }
-
 }
diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/PingFactory.java b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/PingFactory.java
index 285a1fcd57e..c9f722ef79b 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/PingFactory.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/PingFactory.java
@@ -6,7 +6,5 @@ import com.yahoo.search.cluster.ClusterMonitor;
 import java.util.concurrent.Callable;
 
 public interface PingFactory {
-
     Callable<Pong> createPinger(Node node, ClusterMonitor<Node> monitor);
-
 }
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 2bcac6e2ce4..c091e211aca 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
@@ -55,7 +55,7 @@ public class SearchCluster implements NodeManager<Node> {
      * if it only queries this cluster when the local node cannot be used, to avoid unnecessary
      * cross-node network traffic.
      */
-    private final Optional<Node> localCorpusDispatchTarget;
+    private final Optional<Node> directDispatchTarget;
 
     public SearchCluster(String clusterId, DispatchConfig dispatchConfig, int containerClusterSize, VipStatus vipStatus) {
         this.clusterId = clusterId;
@@ -82,11 +82,8 @@ public class SearchCluster implements NodeManager<Node> {
             nodesByHostBuilder.put(node.hostname(), node);
         this.nodesByHost = nodesByHostBuilder.build();
 
-        this.localCorpusDispatchTarget = findLocalCorpusDispatchTarget(HostName.getLocalhost(),
-                                                                       size,
-                                                                       containerClusterSize,
-                                                                       nodesByHost,
-                                                                       groups);
+        this.directDispatchTarget = findDirectDispatchTarget(HostName.getLocalhost(), size, containerClusterSize,
+                                                             nodesByHost, groups);
 
         this.clusterMonitor = new ClusterMonitor<>(this);
     }
@@ -105,11 +102,11 @@ public class SearchCluster implements NodeManager<Node> {
         }
     }
 
-    private static Optional<Node> findLocalCorpusDispatchTarget(String selfHostname,
-                                                                int searchClusterSize,
-                                                                int containerClusterSize,
-                                                                ImmutableMultimap<String, Node> nodesByHost,
-                                                                ImmutableMap<Integer, Group> groups) {
+    private static Optional<Node> findDirectDispatchTarget(String selfHostname,
+                                                           int searchClusterSize,
+                                                           int containerClusterSize,
+                                                           ImmutableMultimap<String, Node> nodesByHost,
+                                                           ImmutableMap<Integer, Group> groups) {
         // A search node in the search cluster in question is configured on the same host as the currently running container.
         // It has all the data <==> No other nodes in the search cluster have the same group id as this node.
         //         That local search node responds.
@@ -189,101 +186,70 @@ public class SearchCluster implements NodeManager<Node> {
     }
 
     /**
-     * Returns the single, local node we should dispatch queries directly to,
+     * Returns the recipient we should dispatch queries directly to (bypassing fdispatch),
      * or empty if we should not dispatch directly.
      */
-    public Optional<Node> localCorpusDispatchTarget() {
-        if ( localCorpusDispatchTarget.isEmpty()) return Optional.empty();
+    public Optional<Node> directDispatchTarget() {
+        if ( directDispatchTarget.isEmpty()) return Optional.empty();
 
         // Only use direct dispatch if the local group has sufficient coverage
-        Group localSearchGroup = groups.get(localCorpusDispatchTarget.get().group());
+        Group localSearchGroup = groups.get(directDispatchTarget.get().group());
         if ( ! localSearchGroup.hasSufficientCoverage()) return Optional.empty();
 
         // Only use direct dispatch if the local search node is up
-        if ( ! localCorpusDispatchTarget.get().isWorking()) return Optional.empty();
+        if ( ! directDispatchTarget.get().isWorking()) return Optional.empty();
 
-        return localCorpusDispatchTarget;
+        return directDispatchTarget;
     }
 
-    /** Called by the cluster monitor whenever we get information (positive or negative) about a node */
-    @Override
-    public void statusIsKnown(Node node) {
-        node.setStatusIsKnown();
-    }
-
-    /** Called by the cluster monitor when node state changes to working */
+    /** Used by the cluster monitor to manage node status */
     @Override
     public void working(Node node) {
         node.setWorking(true);
-        updateVipStatusOnNodeChange(node, true);
+
+        if (usesDirectDispatchTo(node))
+            vipStatus.addToRotation(clusterId);
     }
 
-    /** Called by the cluster monitor when node state changes to failed */
+    /** Used by the cluster monitor to manage node status */
     @Override
     public void failed(Node node) {
         node.setWorking(false);
-        updateVipStatusOnNodeChange(node, true);
-    }
 
-    private void updateSufficientCoverage(Group group, boolean sufficientCoverage) {
-        if (sufficientCoverage == group.hasSufficientCoverage()) return; // no change
-
-        group.setHasSufficientCoverage(sufficientCoverage);
-        updateVipStatusOnCoverageChange(group, sufficientCoverage);
-    }
-
-    private void updateVipStatusOnNodeChange(Node node, boolean working) {
-        if (usesLocalCorpusIn(node)) { // follow the status of the local corpus
-            if (working)
-                vipStatus.addToRotation(clusterId);
-            else
-                vipStatus.removeFromRotation(clusterId);
-        }
-        else {
-            if ( ! hasInformationAboutAllNodes()) return;
-
-            if (hasWorkingNodes())
-                vipStatus.addToRotation(clusterId);
-            else
-                vipStatus.removeFromRotation(clusterId);
-        }
+        // Take ourselves out if we usually dispatch only to our own host
+        if (usesDirectDispatchTo(node))
+            vipStatus.removeFromRotation(clusterId);
     }
 
-    private void updateVipStatusOnCoverageChange(Group group, boolean sufficientCoverage) {
+    private void updateSufficientCoverage(Group group, boolean sufficientCoverage) {
+        // update VIP status if we direct dispatch to this group and coverage status changed
         boolean isInRotation = vipStatus.isInRotation();
-        if (usesLocalCorpusIn(group)) { // follow the status of the local corpus
-            if (sufficientCoverage)
-                vipStatus.addToRotation(clusterId);
-            else
-                vipStatus.removeFromRotation(clusterId);
-        }
-        else {
-            if ( ! isInRotation && sufficientCoverage)
-                vipStatus.addToRotation(clusterId);
+        boolean hasChanged = sufficientCoverage != group.hasSufficientCoverage();
+        boolean isDirectDispatchGroupAndChange = usesDirectDispatchTo(group) && hasChanged;
+        group.setHasSufficientCoverage(sufficientCoverage);
+        if ((!isInRotation || isDirectDispatchGroupAndChange) && sufficientCoverage) {
+            // We will set this cluster in rotation if
+            // - not already in rotation and one group has sufficient coverage.
+            vipStatus.addToRotation(clusterId);
+        } else if (isDirectDispatchGroupAndChange) {
+            // We will take it out of rotation if the group is mandatory (direct dispatch to this group)
+            vipStatus.removeFromRotation(clusterId);
         }
     }
 
-    private boolean hasInformationAboutAllNodes() {
-        return nodesByHost.values().stream().allMatch(Node::getStatusIsKnown);
-    }
-
-    private boolean hasWorkingNodes() {
-        return nodesByHost.values().stream().anyMatch(Node::isWorking);
-    }
-
-    private boolean usesLocalCorpusIn(Node node) {
-        return localCorpusDispatchTarget.isPresent() && localCorpusDispatchTarget.get().equals(node);
+    private boolean usesDirectDispatchTo(Node node) {
+        return directDispatchTarget.isPresent() && directDispatchTarget.get().equals(node);
     }
 
-    private boolean usesLocalCorpusIn(Group group) {
-        return localCorpusDispatchTarget.isPresent() && localCorpusDispatchTarget.get().group() == group.id();
+    private boolean usesDirectDispatchTo(Group group) {
+        return directDispatchTarget.isPresent() && directDispatchTarget.get().group() == group.id();
     }
 
     /** Used by the cluster monitor to manage node status */
     @Override
     public void ping(Node node, Executor executor) {
-        if (pingFactory == null) return; // not initialized yet
-
+        if (pingFactory == null) // not initialized yet
+            return;
         FutureTask<Pong> futurePong = new FutureTask<>(pingFactory.createPinger(node, clusterMonitor));
         executor.execute(futurePong);
         Pong pong = getPong(futurePong, node);
@@ -306,10 +272,9 @@ public class SearchCluster implements NodeManager<Node> {
         // group will always be marked sufficient for use.
         updateSufficientCoverage(group, true);
         boolean fullCoverage = isGroupCoverageSufficient(group.workingNodes(), group.nodes().size(), group.getActiveDocuments(),
-                                                         group.getActiveDocuments());
+                group.getActiveDocuments());
         trackGroupCoverageChanges(0, group, fullCoverage, group.getActiveDocuments());
     }
-
     private void pingIterationCompletedMultipleGroups() {
         int numGroups = orderedGroups.size();
         // Update active documents per group and use it to decide if the group should be active
@@ -334,7 +299,13 @@ public class SearchCluster implements NodeManager<Node> {
             trackGroupCoverageChanges(i, group, sufficientCoverage, averageDocumentsInOtherGroups);
         }
     }
-
+    private boolean areAllNodesDownInAllgroups() {
+        for(int i = 0; i < groups.size(); i++) {
+            Group group = orderedGroups.get(i);
+            if (group.workingNodes() > 0) return false;
+        }
+        return true;
+    }
     /**
      * Update statistics after a round of issuing pings.
      * Note that this doesn't wait for pings to return, so it will typically accumulate data from
@@ -348,6 +319,9 @@ public class SearchCluster implements NodeManager<Node> {
         } else {
             pingIterationCompletedMultipleGroups();
         }
+        if ( areAllNodesDownInAllgroups() ) {
+            vipStatus.removeFromRotation(clusterId);
+        }
     }
 
     private boolean isGroupCoverageSufficient(int workingNodes, int nodesInGroup, long activeDocuments, long averageDocumentsInOtherGroups) {
@@ -423,11 +397,11 @@ public class SearchCluster implements NodeManager<Node> {
         if (changed) {
             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()));
+                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));
+                log.warning(() -> String.format("Coverage of group %d is only %d/%d (requires %d)", index, group.workingNodes(), groupSize(),
+                        requiredNodes));
             }
         }
     }
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..03a417c9bbb 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
@@ -69,7 +69,7 @@ public class DispatcherTest {
     public void requireThatDispatcherSupportsSingleNodeDirectDispatch() {
         SearchCluster cl = new MockSearchCluster("1", 0, 0) {
             @Override
-            public Optional<Node> localCorpusDispatchTarget() {
+            public Optional<Node> directDispatchTarget() {
                 return Optional.of(new Node(1, "test", 123, 1));
             }
         };
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 f4bfa766328..7ee62ae9978 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
@@ -89,7 +89,7 @@ public class MockSearchCluster extends SearchCluster {
     }
 
     @Override
-    public Optional<Node> localCorpusDispatchTarget() {
+    public Optional<Node> directDispatchTarget() {
         return Optional.empty();
     }