Merge pull request #23751 from vespa-engine/hmusum/cleanup-17

Cluster controller unit test cleanup, part 6

9 files changed, 67 insertions, 72 deletions

diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/FleetController.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/FleetController.java
index 2319461ccd6..3f3bf62bf4d 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/FleetController.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/FleetController.java
@@ -1200,7 +1200,7 @@ public class FleetController implements NodeListener, SlobrokListener, SystemSta
             while (true) {
                 int distCount = 0, storCount = 0;
                 for (NodeInfo info : cluster.getNodeInfos()) {
-                    if (!info.isRpcAddressOutdated()) {
+                    if (info.isInSlobrok()) {
                         if (info.isDistributor()) ++distCount;
                         else ++storCount;
                     }
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeInfo.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeInfo.java
index 2993784dba4..746432f1d38 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeInfo.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeInfo.java
@@ -3,7 +3,6 @@ package com.yahoo.vespa.clustercontroller.core;
 
 import com.yahoo.collections.Pair;
 import com.yahoo.jrt.Target;
-import java.util.logging.Level;
 import com.yahoo.vdslib.distribution.Distribution;
 import com.yahoo.vdslib.distribution.Group;
 import com.yahoo.vdslib.state.ClusterState;
@@ -13,12 +12,12 @@ import com.yahoo.vdslib.state.NodeType;
 import com.yahoo.vdslib.state.State;
 import com.yahoo.vespa.clustercontroller.core.hostinfo.HostInfo;
 import com.yahoo.vespa.clustercontroller.core.rpc.RPCCommunicator;
-
 import java.io.PrintWriter;
 import java.io.StringWriter;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.TreeMap;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 
 /**
@@ -244,11 +243,13 @@ abstract public class NodeInfo implements Comparable<NodeInfo> {
 
     public ContentCluster getCluster() { return cluster; }
 
-    /** Returns true if the node is currently registered in slobrok */
-    // FIXME why is this called "isRpcAddressOutdated" then???
-    public boolean isRpcAddressOutdated() { return lastSeenInSlobrok != null; }
+    /** Returns true if the node is registered in slobrok */
+    public boolean isInSlobrok() { return lastSeenInSlobrok == null; }
+
+    /** Returns true if the node is NOT registered in slobrok */
+    public boolean isNotInSlobrok() { return ! isInSlobrok(); }
 
-    public Long getRpcAddressOutdatedTimestamp() { return lastSeenInSlobrok; }
+    public Long lastSeenInSlobrok() { return lastSeenInSlobrok; }
 
     public void abortCurrentNodeStateRequests() {
         for(Pair<GetNodeStateRequest, Long> it : pendingNodeStateRequests) {
@@ -275,7 +276,7 @@ abstract public class NodeInfo implements Comparable<NodeInfo> {
         return wantedState;
     }
 
-    /** Returns the wanted state set directly by a user (i.e not configured) */
+    /** Returns the wanted state set directly by a user (i.e. not configured) */
     public NodeState getUserWantedState() { return wantedState; }
 
     public long getTimeOfFirstFailingConnectionAttempt() {
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateGatherer.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateGatherer.java
index 69e97de84f9..68e46414c22 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateGatherer.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeStateGatherer.java
@@ -3,14 +3,13 @@ package com.yahoo.vespa.clustercontroller.core;
 
 import com.yahoo.jrt.ErrorCode;
 import com.yahoo.jrt.Target;
-import java.util.logging.Level;
 import com.yahoo.vdslib.state.NodeState;
 import com.yahoo.vdslib.state.State;
 import com.yahoo.vespa.clustercontroller.core.hostinfo.HostInfo;
 import com.yahoo.vespa.clustercontroller.core.listeners.NodeListener;
-
 import java.util.LinkedList;
 import java.util.List;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 
 /**
@@ -63,10 +62,10 @@ public class NodeStateGatherer {
             if (requestTime != null && (currentTime - requestTime < nodeStateRequestTimeoutMS)) continue; // pending request
             if (info.getTimeForNextStateRequestAttempt() > currentTime) continue; // too early
 
-            if (info.getRpcAddress() == null || info.isRpcAddressOutdated()) { // Cannot query state of node without RPC address
+            if (info.getRpcAddress() == null || info.isNotInSlobrok()) { // Cannot query state of node without RPC address or not in slobrok
                 log.log(Level.FINE, () -> "Not sending getNodeState request to node " + info.getNode() + ": Not in slobrok");
                 NodeState reportedState = info.getReportedState().clone();
-                if (( ! reportedState.getState().equals(State.DOWN) && currentTime - info.getRpcAddressOutdatedTimestamp() > maxSlobrokDisconnectGracePeriod)
+                if (( ! reportedState.getState().equals(State.DOWN) && currentTime - info.lastSeenInSlobrok() > maxSlobrokDisconnectGracePeriod)
                     || reportedState.getState().equals(State.STOPPING)) // Don't wait for grace period if we expect node to be stopping
                 {
                     log.log(Level.FINE, () -> "Setting reported state to DOWN "
@@ -75,8 +74,8 @@ public class NodeStateGatherer {
                                 : "as node has been out of slobrok longer than " + maxSlobrokDisconnectGracePeriod + "."));
                     if (reportedState.getState().oneOf("iur") || ! reportedState.hasDescription()) {
                         StringBuilder sb = new StringBuilder().append("Set node down as it has been out of slobrok for ")
-                                .append(currentTime - info.getRpcAddressOutdatedTimestamp()).append(" ms which is more than the max limit of ")
-                                .append(maxSlobrokDisconnectGracePeriod).append(" ms.");
+                                                              .append(currentTime - info.lastSeenInSlobrok()).append(" ms which is more than the max limit of ")
+                                                              .append(maxSlobrokDisconnectGracePeriod).append(" ms.");
                         reportedState.setDescription(sb.toString());
                     }
                     reportedState.setState(State.DOWN);
@@ -181,7 +180,7 @@ public class NodeStateGatherer {
                     newState.setState(State.DOWN);
                 } else if (msg.equals("jrt: Connection closed by peer") || msg.equals("Connection reset by peer")) {
                     msg = "Connection error: Closed at other end. (Node or switch likely shut down)";
-                    if (info.isRpcAddressOutdated()) {
+                    if (info.isNotInSlobrok()) {
                         msg += " Node is no longer in slobrok.";
                     }
                     if (info.getReportedState().getState().oneOf("ui")) {
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/StateChangeHandler.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/StateChangeHandler.java
index 4c832592422..9897e3cf04c 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/StateChangeHandler.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/StateChangeHandler.java
@@ -329,7 +329,7 @@ public class StateChangeHandler {
     {
         return currentStateInSystem.getState().equals(State.MAINTENANCE)
             && node.getWantedState().above(new NodeState(node.getNode().getType(), State.DOWN))
-            && (lastReportedState.getState().equals(State.DOWN) || node.isRpcAddressOutdated())
+            && (lastReportedState.getState().equals(State.DOWN) || node.isNotInSlobrok())
             && node.getTransitionTime() + maxTransitionTime.get(node.getNode().getType()) < currentTime;
     }
 
@@ -339,14 +339,14 @@ public class StateChangeHandler {
                                                     NodeInfo node,
                                                     NodeState lastReportedState)
     {
-        if (node.isRpcAddressOutdated()
+        if (node.isNotInSlobrok()
             && !lastReportedState.getState().equals(State.DOWN)
-            && node.getRpcAddressOutdatedTimestamp() + maxSlobrokDisconnectGracePeriod <= currentTime)
+            && node.lastSeenInSlobrok() + maxSlobrokDisconnectGracePeriod <= currentTime)
         {
             final String desc = String.format(
                     "Set node down as it has been out of slobrok for %d ms which " +
                     "is more than the max limit of %d ms.",
-                    currentTime - node.getRpcAddressOutdatedTimestamp(),
+                    currentTime - node.lastSeenInSlobrok(),
                     maxSlobrokDisconnectGracePeriod);
             node.abortCurrentNodeStateRequests();
             NodeState state = lastReportedState.clone();
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/SystemStateBroadcaster.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/SystemStateBroadcaster.java
index 2359e4d8389..004cefe1e3c 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/SystemStateBroadcaster.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/SystemStateBroadcaster.java
@@ -161,7 +161,7 @@ public class SystemStateBroadcaster {
     }
 
     private static boolean nodeIsReachable(NodeInfo node) {
-        if (node.getRpcAddress() == null || node.isRpcAddressOutdated()) {
+        if (node.getRpcAddress() == null || node.isNotInSlobrok()) {
             return false; // Can't set state on nodes we don't know where are
         }
         if (node.getReportedState().getState() == State.MAINTENANCE ||
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RpcServer.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RpcServer.java
index dc21693dcdb..06f6777ab80 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RpcServer.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RpcServer.java
@@ -199,7 +199,7 @@ public class RpcServer {
                 if (!e.getMessage().equals(lastConnectError) || time - lastConnectErrorTime > 60 * 1000) {
                     lastConnectError = e.getMessage();
                     lastConnectErrorTime = time;
-                    log.log(Level.WARNING, "Failed to initailize RPC server socket: " + e.getMessage());
+                    log.log(Level.WARNING, "Failed to initialize RPC server socket: " + e.getMessage());
                 }
             }
         }
@@ -227,8 +227,8 @@ public class RpcServer {
                 }
                 if (req.methodName().equals("getNodeList")) {
                     log.log(Level.FINE, "Resolving RPC getNodeList request");
-                    List<String> slobrok = new ArrayList<String>();
-                    List<String> rpc = new ArrayList<String>();
+                    List<String> slobrok = new ArrayList<>();
+                    List<String> rpc = new ArrayList<>();
                     for(NodeInfo node : cluster.getNodeInfos()) {
                         String s1 = node.getSlobrokAddress();
                         String s2 = node.getRpcAddress();
@@ -236,8 +236,8 @@ public class RpcServer {
                         slobrok.add(s1);
                         rpc.add(s2 == null ? "" : s2);
                     }
-                    req.returnValues().add(new StringArray(slobrok.toArray(new String[slobrok.size()])));
-                    req.returnValues().add(new StringArray(rpc.toArray(new String[rpc.size()])));
+                    req.returnValues().add(new StringArray(slobrok.toArray(new String[0])));
+                    req.returnValues().add(new StringArray(rpc.toArray(new String[0])));
                     req.returnRequest();
                 } else if (req.methodName().equals("getSystemState")) {
                     log.log(Level.FINE, "Resolving RPC getSystemState request");
@@ -280,7 +280,7 @@ public class RpcServer {
                     NodeState oldState = node.getUserWantedState();
                     String message = (nodeState.getState().equals(State.UP)
                             ? "Clearing wanted nodeState for node " + node
-                            : "New wantedstate '" + nodeState.toString() + "' stored for node " + node);
+                            : "New wantedstate '" + nodeState + "' stored for node " + node);
                     if (!oldState.equals(nodeState) || !oldState.getDescription().equals(nodeState.getDescription())) {
                         if (!nodeState.getState().validWantedNodeState(nodeType)) {
                             throw new IllegalStateException("State " + nodeState.getState()
@@ -289,7 +289,7 @@ public class RpcServer {
                         node.setWantedState(nodeState);
                         changeListener.handleNewWantedNodeState(node, nodeState);
                     } else {
-                        message = "Node " + node + " already had wanted state " + nodeState.toString();
+                        message = "Node " + node + " already had wanted state " + nodeState;
                         log.log(Level.FINE, message);
                     }
                     req.returnValues().add(new StringValue(message));
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/SlobrokClient.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/SlobrokClient.java
index c88bf71af09..8393e776fc2 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/SlobrokClient.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/SlobrokClient.java
@@ -150,7 +150,7 @@ public class SlobrokClient implements NodeLookup {
         }
         cluster.setSlobrokGenerationCount(mirrorVersion);
         for (NodeInfo nodeInfo : cluster.getNodeInfos()) {
-            if (slobrokNodes.containsKey(nodeInfo.getNode()) && nodeInfo.isRpcAddressOutdated()) {
+            if (slobrokNodes.containsKey(nodeInfo.getNode()) && nodeInfo.isNotInSlobrok()) {
                 context.log(log,
                             Level.WARNING,
                             "Node " + nodeInfo
@@ -187,7 +187,7 @@ public class SlobrokClient implements NodeLookup {
                 newNext = null;
             } else if (newNext == null || newNext.node.compareTo(oldNext.getNode()) > 0) {
                 assert(slobrokNodes.get(oldNext.getNode()) == null);
-                if (!oldNext.isRpcAddressOutdated() && oldNext.getRpcAddress() != null) {
+                if (oldNext.isInSlobrok() && oldNext.getRpcAddress() != null) {
                     missingNodeInfos.add(oldNext);
                 }
                 oldNext = null;
@@ -195,7 +195,7 @@ public class SlobrokClient implements NodeLookup {
                 assert(newNext.rpcAddress != null);
                 if (oldNext.getRpcAddress() == null || !oldNext.getRpcAddress().equals(newNext.rpcAddress)) {
                     alteredRpcAddress.add(newNext);
-                } else if (oldNext.isRpcAddressOutdated()) {
+                } else if (oldNext.isNotInSlobrok()) {
                     returningRpcAddressNodeInfos.add(oldNext);
                 }
                 oldNext = null;
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/statuspage/VdsClusterHtmlRenderer.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/statuspage/VdsClusterHtmlRenderer.java
index 1081f3e77cd..d840529d361 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/statuspage/VdsClusterHtmlRenderer.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/statuspage/VdsClusterHtmlRenderer.java
@@ -243,7 +243,7 @@ public class VdsClusterHtmlRenderer {
                 row.addCell(new HtmlTable.Cell("-").addProperties(ERROR_PROPERTY));
             } else {
                 row.addCell(new HtmlTable.Cell(HtmlTable.escape(nodeInfo.getRpcAddress())));
-                if (nodeInfo.isRpcAddressOutdated()) {
+                if (nodeInfo.isNotInSlobrok()) {
                     row.getLastCell().addProperties(WARNING_PROPERTY);
                 }
             }
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/testutils/WaitCondition.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/testutils/WaitCondition.java
index d6d3e2876a5..deccaf282d6 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/testutils/WaitCondition.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/testutils/WaitCondition.java
@@ -9,11 +9,10 @@ import com.yahoo.vespa.clustercontroller.core.ClusterStateBundle;
 import com.yahoo.vespa.clustercontroller.core.DummyVdsNode;
 import com.yahoo.vespa.clustercontroller.core.FleetController;
 import com.yahoo.vespa.clustercontroller.core.listeners.SystemStateListener;
-
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.List;
+import java.util.Objects;
 import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -65,10 +64,10 @@ public interface WaitCondition {
     class RegexStateMatcher extends StateWait {
 
         private final Pattern pattern;
-        private Collection<DummyVdsNode> nodesToCheck;
+        private Collection<DummyVdsNode> nodesToCheck = Set.of();
         private ClusterState lastCheckedState;
         private boolean checkAllSpaces = false;
-        private Set<String> checkSpaceSubset = Collections.emptySet();
+        private Set<String> checkSpaceSubset = Set.of();
 
         RegexStateMatcher(String regex, FleetController fc, Object monitor) {
             super(fc, monitor);
@@ -76,6 +75,7 @@ public interface WaitCondition {
         }
 
         RegexStateMatcher includeNotifyingNodes(Collection<DummyVdsNode> nodes) {
+            Objects.requireNonNull(nodes, "nodes must be non-null");
             nodesToCheck = nodes;
             return this;
         }
@@ -86,6 +86,7 @@ public interface WaitCondition {
         }
 
         RegexStateMatcher checkSpaceSubset(Set<String> spaces) {
+            Objects.requireNonNull(spaces, "spaces must be non-null");
             this.checkSpaceSubset = spaces;
             return this;
         }
@@ -99,45 +100,39 @@ public interface WaitCondition {
 
         @Override
         public String isConditionMet() {
-            if (convergedState != null) {
-                lastCheckedState = convergedState;
-                Matcher m = pattern.matcher(lastCheckedState.toString());
-                if (m.matches() || !checkSpaceSubset.isEmpty()) {
-                    if (nodesToCheck != null) {
-                        for (DummyVdsNode node : nodesToCheck) {
-                            if (node.getClusterState() == null) {
-                                return "Node " + node + " has not received a cluster state yet";
-                            }
-                            // TODO refactor, simplify
-                            boolean match;
-                            if (checkAllSpaces) {
-                                match = statesInBundle(node.getClusterStateBundle()).stream()
-                                        .allMatch(state -> pattern.matcher(withoutTimestamps(state.toString())).matches());
-                            } else if (!checkSpaceSubset.isEmpty()) {
-                                match = checkSpaceSubset.stream().allMatch(space -> {
-                                    String state = node.getClusterStateBundle().getDerivedBucketSpaceStates()
-                                            .getOrDefault(space, AnnotatedClusterState.emptyState()).getClusterState().toString();
-                                    return pattern.matcher(withoutTimestamps(state)).matches();
-                                });
-                            } else {
-                                match = pattern.matcher(withoutTimestamps(node.getClusterState().toString())).matches();
-                            }
-
-                            if (!match) {
-                                return "Node " + node + " state mismatch.\n  wanted: " + pattern + "\n  is:     " + node.getClusterStateBundle().toString();
-                            }
-                            if (node.getStateCommunicationVersion() > 0) {
-                                if (!node.hasPendingGetNodeStateRequest()) {
-                                    return "Node " + node + " has not received another get node state request yet";
-                                }
-                            }
-                        }
-                    }
-                    return null;
+            if (convergedState == null) return "No cluster state defined yet";
+
+            lastCheckedState = convergedState;
+            Matcher m = pattern.matcher(lastCheckedState.toString());
+            if (!m.matches() && checkSpaceSubset.isEmpty()) return "Cluster state mismatch";
+
+            for (DummyVdsNode node : nodesToCheck) {
+                if (node.getClusterState() == null) return "Node " + node + " has not received a cluster state yet";
+
+                boolean match;
+                if (checkAllSpaces) {
+                    match = statesInBundle(node.getClusterStateBundle()).stream()
+                                                                        .allMatch(state -> pattern
+                                                                                .matcher(withoutTimestamps(state.toString()))
+                                                                                .matches());
+                } else if (!checkSpaceSubset.isEmpty()) {
+                    match = checkSpaceSubset.stream().allMatch(space -> {
+                        String state = node.getClusterStateBundle().getDerivedBucketSpaceStates()
+                                           .getOrDefault(space, AnnotatedClusterState.emptyState()).getClusterState().toString();
+                        return pattern.matcher(withoutTimestamps(state)).matches();
+                    });
+                } else {
+                    match = pattern.matcher(withoutTimestamps(node.getClusterState().toString())).matches();
+                }
+
+                if (!match) {
+                    return "Node " + node + " state mismatch.\n  wanted: " + pattern + "\n  is:     " + node.getClusterStateBundle().toString();
+                }
+                if (node.getStateCommunicationVersion() > 0 && !node.hasPendingGetNodeStateRequest()) {
+                    return "Node " + node + " has not received another get node state request yet";
                 }
-                return "Cluster state mismatch";
             }
-            return "No cluster state defined yet";
+            return null;
         }
 
         /** Returns the given state string with timestamps removed */