Add method to use Duration as timeout to make unit explicit, and avoid many magic conversions.

6 files changed, 61 insertions, 62 deletions

diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicator.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicator.java
index 502fc37dead..e223ad12fb9 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicator.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicator.java
@@ -22,6 +22,8 @@ import com.yahoo.vespa.clustercontroller.core.GetNodeStateRequest;
 import com.yahoo.vespa.clustercontroller.core.NodeInfo;
 import com.yahoo.vespa.clustercontroller.core.SetClusterStateRequest;
 import com.yahoo.vespa.clustercontroller.core.Timer;
+
+import java.time.Duration;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -46,10 +48,10 @@ public class RPCCommunicator implements Communicator {
 
     private final Timer timer;
     private final Supervisor supervisor;
-    private double nodeStateRequestTimeoutIntervalMaxSeconds;
+    private Duration nodeStateRequestTimeoutIntervalMax;
     private int nodeStateRequestTimeoutIntervalStartPercentage;
     private int nodeStateRequestTimeoutIntervalStopPercentage;
-    private int nodeStateRequestRoundTripTimeMaxSeconds;
+    private Duration nodeStateRequestRoundTripTimeMax;
     private final int fleetControllerIndex;
 
     public static Supervisor createRealSupervisor() {
@@ -72,10 +74,10 @@ public class RPCCommunicator implements Communicator {
         checkArgument(nodeStateRequestTimeoutIntervalStopPercentage >= nodeStateRequestTimeoutIntervalStartPercentage);
         checkArgument(nodeStateRequestTimeoutIntervalStopPercentage <= 100);
         checkArgument(nodeStateRequestRoundTripTimeMaxSeconds >= 0);
-        this.nodeStateRequestTimeoutIntervalMaxSeconds = nodeStateRequestTimeoutIntervalMaxMs / 1000D;
+        this.nodeStateRequestTimeoutIntervalMax = Duration.ofMillis(nodeStateRequestTimeoutIntervalMaxMs);
         this.nodeStateRequestTimeoutIntervalStartPercentage = nodeStateRequestTimeoutIntervalStartPercentage;
         this.nodeStateRequestTimeoutIntervalStopPercentage = nodeStateRequestTimeoutIntervalStopPercentage;
-        this.nodeStateRequestRoundTripTimeMaxSeconds = nodeStateRequestRoundTripTimeMaxSeconds;
+        this.nodeStateRequestRoundTripTimeMax = Duration.ofSeconds(nodeStateRequestRoundTripTimeMaxSeconds);
         this.supervisor = supervisor;
     }
 
@@ -100,10 +102,10 @@ public class RPCCommunicator implements Communicator {
                       >= options.nodeStateRequestTimeoutEarliestPercentage);
         checkArgument(options.nodeStateRequestTimeoutLatestPercentage <= 100);
         checkArgument(options.nodeStateRequestRoundTripTimeMaxSeconds >= 0);
-        this.nodeStateRequestTimeoutIntervalMaxSeconds = options.nodeStateRequestTimeoutMS / 1000.0;
+        this.nodeStateRequestTimeoutIntervalMax = Duration.ofMillis(options.nodeStateRequestTimeoutMS);
         this.nodeStateRequestTimeoutIntervalStartPercentage = options.nodeStateRequestTimeoutEarliestPercentage;
         this.nodeStateRequestTimeoutIntervalStopPercentage = options.nodeStateRequestTimeoutLatestPercentage;
-        this.nodeStateRequestRoundTripTimeMaxSeconds = options.nodeStateRequestRoundTripTimeMaxSeconds;
+        this.nodeStateRequestRoundTripTimeMax = Duration.ofSeconds(options.nodeStateRequestRoundTripTimeMaxSeconds);
     }
 
     @Override
@@ -117,16 +119,15 @@ public class RPCCommunicator implements Communicator {
         req.parameters().add(new StringValue(
                 currentState.getState().equals(State.DOWN) || node.getConnectionAttemptCount() > 0
                    ? "unknown" : currentState.serialize()));
-        req.parameters().add(new Int32Value(generateNodeStateRequestTimeoutMs()));
+        req.parameters().add(new Int32Value((int)generateNodeStateRequestTimeout().toMillis()));
         req.parameters().add(new Int32Value(fleetControllerIndex));
 
         RPCGetNodeStateRequest stateRequest = new RPCGetNodeStateRequest(node, req);
         RPCGetNodeStateWaiter waiter = new RPCGetNodeStateWaiter(stateRequest, externalWaiter, timer);
 
-        double requestTimeoutSeconds =
-            nodeStateRequestTimeoutIntervalMaxSeconds + nodeStateRequestRoundTripTimeMaxSeconds;
+        Duration requestTimeout = nodeStateRequestTimeoutIntervalMax.plus(nodeStateRequestRoundTripTimeMax);
 
-        connection.invokeAsync(req, requestTimeoutSeconds, waiter);
+        connection.invokeAsync(req, requestTimeout, waiter);
         node.setCurrentNodeStateRequest(stateRequest, timer.getCurrentTimeInMillis());
         node.lastRequestInfoConnection = connection;
     }
@@ -161,7 +162,7 @@ public class RPCCommunicator implements Communicator {
         RPCSetClusterStateRequest stateRequest = new RPCSetClusterStateRequest(node, req, baselineState.getVersion());
         waiter.setRequest(stateRequest);
 
-        connection.invokeAsync(req, 60, waiter);
+        connection.invokeAsync(req, Duration.ofSeconds(60), waiter);
         node.setClusterStateVersionBundleSent(stateBundle);
     }
 
@@ -183,20 +184,20 @@ public class RPCCommunicator implements Communicator {
         var activationRequest = new RPCActivateClusterStateVersionRequest(node, req, clusterStateVersion);
         waiter.setRequest(activationRequest);
 
-        connection.invokeAsync(req, 60, waiter);
+        connection.invokeAsync(req, Duration.ofSeconds(60), waiter);
         node.setClusterStateVersionActivationSent(clusterStateVersion);
     }
 
     // protected for testing.
-    protected int generateNodeStateRequestTimeoutMs() {
+    protected Duration generateNodeStateRequestTimeout() {
         double intervalFraction = Math.random();
-        double earliestTimeoutSeconds =
-                nodeStateRequestTimeoutIntervalMaxSeconds * nodeStateRequestTimeoutIntervalStartPercentage / 100.0;
-        double latestTimeoutSeconds =
-                nodeStateRequestTimeoutIntervalMaxSeconds * nodeStateRequestTimeoutIntervalStopPercentage / 100.0;
-        double interval = latestTimeoutSeconds - earliestTimeoutSeconds;
-        double timeoutSeconds = earliestTimeoutSeconds + intervalFraction * interval;
-        return (int) (timeoutSeconds * 1000);
+        long earliestTimeoutNanos =
+                nodeStateRequestTimeoutIntervalMax.toNanos() * nodeStateRequestTimeoutIntervalStartPercentage / 100;
+        long latestTimeoutNanos =
+                nodeStateRequestTimeoutIntervalMax.toNanos() * nodeStateRequestTimeoutIntervalStopPercentage / 100;
+        long interval = latestTimeoutNanos - earliestTimeoutNanos;
+        long timeoutNanos = earliestTimeoutNanos + (long)(intervalFraction * interval);
+        return Duration.ofNanos(timeoutNanos);
     }
 
 }
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DatabaseTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DatabaseTest.java
index 42bcfaac477..9a6a9e063ac 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DatabaseTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/DatabaseTest.java
@@ -154,7 +154,7 @@ public class DatabaseTest extends FleetControllerTest {
         Request req = new Request("setNodeState");
         req.parameters().add(new StringValue("storage/cluster.mycluster/" + n.getType().toString() + "/" + n.getIndex()));
         req.parameters().add(new StringValue(ns.serialize(true)));
-        connection.invokeSync(req, timeoutInSeconds());
+        connection.invokeSync(req, timeout());
         assertEquals(ErrorCode.NONE, req.errorCode(), req.toString());
         assertTrue(req.checkReturnTypes("s"), req.toString());
         wantedStates.put(n, ns);
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/FleetControllerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/FleetControllerTest.java
index 41ae2b37411..371a57e08c0 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/FleetControllerTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/FleetControllerTest.java
@@ -460,7 +460,7 @@ public abstract class FleetControllerTest implements Waiter {
         Request req = new Request("setNodeState");
         req.parameters().add(new StringValue(node.getSlobrokName()));
         req.parameters().add(new StringValue(ns.serialize()));
-        connection.invokeSync(req, timeoutInSeconds());
+        connection.invokeSync(req, timeout());
         if (req.isError()) {
             fail("Failed to invoke setNodeState(): " + req.errorCode() + ": " + req.errorMessage());
         }
@@ -477,6 +477,4 @@ public abstract class FleetControllerTest implements Waiter {
 
     Duration timeout() { return timeout; }
 
-    double timeoutInSeconds() { return (double) timeout.toMillis() / 1000; }
-
 }
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MasterElectionTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MasterElectionTest.java
index 05a06a92e27..16a3e41f149 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MasterElectionTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MasterElectionTest.java
@@ -313,7 +313,7 @@ public class MasterElectionTest extends FleetControllerTest {
             boolean allOk = true;
             for (int node : nodes) {
                 Request req = new Request("getMaster");
-                connections.get(node).invokeSync(req, timeoutInSeconds());
+                connections.get(node).invokeSync(req, timeout());
                 if (req.isError()) {
                     allOk = false;
                     break;
@@ -355,10 +355,11 @@ public class MasterElectionTest extends FleetControllerTest {
 
         Request req = new Request("getMaster");
 
+        long maxRetries = timeout().toMillis() / 100;
         for (int nodeIndex = 0; nodeIndex < 3; ++nodeIndex) {
-            for (int retry = 0; retry < timeoutInSeconds() * 10; ++retry) {
+            for (int retry = 0; retry < maxRetries; ++retry) {
                 req = new Request("getMaster");
-                connections.get(nodeIndex).invokeSync(req, timeoutInSeconds());
+                connections.get(nodeIndex).invokeSync(req, timeout());
                 assertFalse(req.isError(), req.errorMessage());
                 if (req.returnValues().get(0).asInt32() == 0 &&
                         req.returnValues().get(1).asString().equals("All 3 nodes agree that 0 is current master.")) {
@@ -389,13 +390,13 @@ public class MasterElectionTest extends FleetControllerTest {
         waitForMaster(1);
 
         req = new Request("getMaster");
-        connections.get(0).invokeSync(req, timeoutInSeconds());
+        connections.get(0).invokeSync(req, timeout());
         assertEquals(104, req.errorCode(), req.toString());
         assertEquals("Connection error", req.errorMessage(), req.toString());
 
-        for (int i = 0; i < timeoutInSeconds() * 10; ++i) {
+        for (int i = 0; i < maxRetries; ++i) {
             req = new Request("getMaster");
-            connections.get(1).invokeSync(req, timeoutInSeconds());
+            connections.get(1).invokeSync(req, timeout());
             assertFalse(req.isError(), req.errorMessage());
             if (req.returnValues().get(0).asInt32() != -1) break;
             // We may have bad timing causing node not to have realized it is master yet
@@ -403,9 +404,9 @@ public class MasterElectionTest extends FleetControllerTest {
         assertEquals(1, req.returnValues().get(0).asInt32(), req.toString());
         assertEquals("2 of 3 nodes agree 1 is master.", req.returnValues().get(1).asString(), req.toString());
 
-        for (int i = 0; i < timeoutInSeconds() * 10; ++i) {
+        for (int i = 0; i < maxRetries; ++i) {
             req = new Request("getMaster");
-            connections.get(2).invokeSync(req, timeoutInSeconds());
+            connections.get(2).invokeSync(req, timeout());
             assertFalse(req.isError(), req.errorMessage());
             if (req.returnValues().get(0).asInt32() != -1) break;
         }
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcServerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcServerTest.java
index 594a681f4b5..641f15467a6 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcServerTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/RpcServerTest.java
@@ -127,7 +127,7 @@ public class RpcServerTest extends FleetControllerTest {
         assertTrue(connection.isValid());
 
         Request req = new Request("getSystemState");
-        connection.invokeSync(req, timeoutInSeconds());
+        connection.invokeSync(req, timeout());
         assertEquals(ErrorCode.NONE, req.errorCode(), req.toString());
         assertTrue(req.checkReturnTypes("ss"), req.toString());
         String systemState = req.returnValues().get(1).asString();
@@ -523,7 +523,7 @@ public class RpcServerTest extends FleetControllerTest {
         assertTrue(connection.isValid());
 
         Request req = new Request("getMaster");
-        connection.invokeSync(req, timeoutInSeconds());
+        connection.invokeSync(req, timeout());
         assertEquals(0, req.returnValues().get(0).asInt32(), req.toString());
         assertEquals("All 1 nodes agree that 0 is current master.", req.returnValues().get(1).asString(), req.toString());
 
@@ -548,7 +548,7 @@ public class RpcServerTest extends FleetControllerTest {
         assertTrue(connection.isValid());
 
         Request req = new Request("getNodeList");
-        connection.invokeSync(req, timeoutInSeconds());
+        connection.invokeSync(req, timeout());
         assertEquals(ErrorCode.NONE, req.errorCode(), req.errorMessage());
         assertTrue(req.checkReturnTypes("SS"), req.toString());
         String[] slobrok = req.returnValues().get(0).asStringArray().clone();
@@ -570,7 +570,7 @@ public class RpcServerTest extends FleetControllerTest {
             Request req2 = new Request("getnodestate2");
             req2.parameters().add(new StringValue("unknown"));
             Target connection2 = supervisor.connect(new Spec(rpc[i]));
-            connection2.invokeSync(req2, timeoutInSeconds());
+            connection2.invokeSync(req2, timeout());
             assertEquals(ErrorCode.NONE, req.errorCode(), req2.toString());
         }
     }
@@ -583,7 +583,7 @@ public class RpcServerTest extends FleetControllerTest {
         Request req = new Request("setNodeState");
         req.parameters().add(new StringValue(node));
         req.parameters().add(new StringValue(newNodeState));
-        connection.invokeSync(req, timeoutInSeconds());
+        connection.invokeSync(req, timeout());
         return req;
     }
 
@@ -591,7 +591,7 @@ public class RpcServerTest extends FleetControllerTest {
         Request req = new Request("getNodeState");
         req.parameters().add(new StringValue(nodeType));
         req.parameters().add(new Int32Value(nodeIndex));
-        connection.invokeSync(req, timeoutInSeconds());
+        connection.invokeSync(req, timeout());
         return req;
     }
 
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicatorTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicatorTest.java
index 809979620c2..b9a9d7fbf8f 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicatorTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/rpc/RPCCommunicatorTest.java
@@ -24,15 +24,14 @@ import com.yahoo.vespa.clustercontroller.core.Timer;
 import org.junit.jupiter.api.Test;
 import org.mockito.Mockito;
 
+import java.time.Duration;
 import java.util.HashSet;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicReference;
 
-import static org.hamcrest.CoreMatchers.equalTo;
-import static org.hamcrest.CoreMatchers.notNullValue;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.core.Is.is;
-import static org.hamcrest.core.IsNot.not;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyDouble;
@@ -60,18 +59,18 @@ public class RPCCommunicatorTest {
                 NODE_STATE_REQUEST_TIMEOUT_INTERVAL_START_PERCENTAGE,
                 NODE_STATE_REQUEST_TIMEOUT_INTERVAL_STOP_PERCENTAGE,
                 0);
-        int max = -1;
-        int min = 100000;
-        final Set<Integer> uniqueTimeoutValues = new HashSet<>();
+        long max = -1;
+        long min = 100000;
+        final Set<Long> uniqueTimeoutValues = new HashSet<>();
         for (int x = 0; x < TEST_ITERATIONS; x++) {
-            int timeOutMs = communicator.generateNodeStateRequestTimeoutMs();
+            long timeOutMs = communicator.generateNodeStateRequestTimeout().toMillis();
             min = Math.min(min, timeOutMs);
             max = Math.max(max, timeOutMs);
             uniqueTimeoutValues.add(timeOutMs);
         }
         assertTrue(max <=  NODE_STATE_REQUEST_TIMEOUT_INTERVAL_MAX_MS *
                 NODE_STATE_REQUEST_TIMEOUT_INTERVAL_STOP_PERCENTAGE / 100.);
-        assertThat(min, is(not(max)));
+        assertNotEquals(min, max);
         assertTrue(min >= NODE_STATE_REQUEST_TIMEOUT_INTERVAL_START_PERCENTAGE *
                 NODE_STATE_REQUEST_TIMEOUT_INTERVAL_MAX_MS / 100);
         assertTrue(uniqueTimeoutValues.size() > TEST_ITERATIONS / 2);
@@ -85,8 +84,8 @@ public class RPCCommunicatorTest {
         fleetControllerOptions.nodeStateRequestTimeoutLatestPercentage = 100;
         fleetControllerOptions.nodeStateRequestTimeoutMS = NODE_STATE_REQUEST_TIMEOUT_INTERVAL_MAX_MS;
         communicator.propagateOptions(fleetControllerOptions);
-        int timeOutMs = communicator.generateNodeStateRequestTimeoutMs();
-        assertThat(timeOutMs, is(NODE_STATE_REQUEST_TIMEOUT_INTERVAL_MAX_MS));
+        long timeOutMs = communicator.generateNodeStateRequestTimeout().toMillis();
+        assertEquals(timeOutMs, NODE_STATE_REQUEST_TIMEOUT_INTERVAL_MAX_MS);
     }
 
     @Test
@@ -111,7 +110,7 @@ public class RPCCommunicatorTest {
         communicator.getNodeState(nodeInfo, null);
         Mockito.verify(target).invokeAsync(
                 any(),
-                eq(ROUNDTRIP_LATENCY_SECONDS + NODE_STATE_REQUEST_TIMEOUT_INTERVAL_MAX_MS / 1000.0),
+                eq(Duration.ofSeconds(ROUNDTRIP_LATENCY_SECONDS).plusMillis(NODE_STATE_REQUEST_TIMEOUT_INTERVAL_MAX_MS)),
                 any());
     }
 
@@ -141,7 +140,7 @@ public class RPCCommunicatorTest {
                 receivedRequest.set((Request) invocation.getArguments()[0]);
                 receivedWaiter.set((RequestWaiter) invocation.getArguments()[2]);
                 return null;
-            }).when(mockTarget).invokeAsync(any(), anyDouble(), any());
+            }).when(mockTarget).invokeAsync(any(), any(Duration.class), any());
         }
     }
 
@@ -153,12 +152,12 @@ public class RPCCommunicatorTest {
         f.communicator.setSystemState(sentBundle, cf.cluster().getNodeInfo(Node.ofStorage(1)), f.mockWaiter);
 
         Request req = f.receivedRequest.get();
-        assertThat(req, notNullValue());
-        assertThat(req.methodName(), equalTo(RPCCommunicator.SET_DISTRIBUTION_STATES_RPC_METHOD_NAME));
+        assertNotNull(req);
+        assertEquals(req.methodName(), RPCCommunicator.SET_DISTRIBUTION_STATES_RPC_METHOD_NAME);
         assertTrue(req.parameters().satisfies("bix")); // <compression type>, <uncompressed size>, <payload>
 
         ClusterStateBundle receivedBundle = RPCUtil.decodeStateBundleFromSetDistributionStatesRequest(req);
-        assertThat(receivedBundle, equalTo(sentBundle));
+        assertEquals(receivedBundle, sentBundle);
     }
 
     @Test
@@ -169,9 +168,9 @@ public class RPCCommunicatorTest {
         f.communicator.setSystemState(sentBundle, cf.cluster().getNodeInfo(Node.ofStorage(1)), f.mockWaiter);
 
         RequestWaiter waiter = f.receivedWaiter.get();
-        assertThat(waiter, notNullValue());
+        assertNotNull(waiter);
         Request req = f.receivedRequest.get();
-        assertThat(req, notNullValue());
+        assertNotNull(req);
 
         req.setError(ErrorCode.NO_SUCH_METHOD, "que?");
         waiter.handleRequestDone(req);
@@ -183,8 +182,8 @@ public class RPCCommunicatorTest {
         // Now when we try again, we should have been downgraded to the legacy setsystemstate2 RPC
         f.communicator.setSystemState(sentBundle, cf.cluster().getNodeInfo(Node.ofStorage(1)), f.mockWaiter);
         req = f.receivedRequest.get();
-        assertThat(req, notNullValue());
-        assertThat(req.methodName(), equalTo(RPCCommunicator.LEGACY_SET_SYSTEM_STATE2_RPC_METHOD_NAME));
+        assertNotNull(req);
+        assertEquals(req.methodName(), RPCCommunicator.LEGACY_SET_SYSTEM_STATE2_RPC_METHOD_NAME);
     }
 
     @Test
@@ -194,10 +193,10 @@ public class RPCCommunicatorTest {
         f.communicator.activateClusterStateVersion(12345, cf.cluster().getNodeInfo(Node.ofDistributor(1)), f.mockWaiter);
 
         Request req = f.receivedRequest.get();
-        assertThat(req, notNullValue());
-        assertThat(req.methodName(), equalTo(RPCCommunicator.ACTIVATE_CLUSTER_STATE_VERSION_RPC_METHOD_NAME));
+        assertNotNull(req);
+        assertEquals(req.methodName(), RPCCommunicator.ACTIVATE_CLUSTER_STATE_VERSION_RPC_METHOD_NAME);
         assertTrue(req.parameters().satisfies("i")); // <cluster state version>
-        assertThat(req.parameters().get(0).asInt32(), equalTo(12345));
+        assertEquals(req.parameters().get(0).asInt32(), 12345);
     }
 
 }