ywcb00 commented on code in PR #2587:
URL: https://github.com/apache/systemds/pull/2587#discussion_r3827919900
##########
src/test/java/org/apache/sysds/test/FederatedWorkerUtils.java:
##########
@@ -174,16 +180,33 @@ public static void waitForWorkers(int[] ports, int
timeoutMs, java.util.function
}
}
- private static boolean tryConnect(int port) {
+ private static boolean tryConnect(int port, long deadline) {
+ final int timeout = attemptTimeout(deadline -
System.currentTimeMillis());
+ if(timeout == 0) // out of time, do not start another attempt
+ return false;
try(Socket s = new Socket()) {
- s.connect(new InetSocketAddress("localhost", port),
CONNECT_TIMEOUT_MS);
+ s.connect(new InetSocketAddress("localhost", port),
timeout);
return true;
}
- catch(IOException e) {
+ catch(IOException e) { // closed port, or a handshake that
outlasted the budget
return false;
}
}
+ /**
+ * Budget for a single connect attempt, capped by the time left until
the overall deadline so that one slow attempt
+ * cannot substantially exceed the limit.
+ *
+ * @param remainingMs time left until the deadline, in ms
+ * @return the timeout to pass to {@link Socket#connect}, or 0 if no
attempt should be made. Never returns 0 while
+ * time is left, because {@code connect} reads a timeout of 0
as 'infinite'.
+ */
+ public static int attemptTimeout(long remainingMs) {
Review Comment:
This name here is ambiguous. While this method returns the timeout for a
connection attempt, the name could also mean that it attempts the connection
with a timeout.
##########
src/test/java/org/apache/sysds/test/FederatedWorkerUtils.java:
##########
@@ -25,20 +25,24 @@
import java.util.function.BooleanSupplier;
/**
- * Test helpers that block until a federated worker is accepting TCP
connections on its port.
- *
- * <p>The federated worker opens its TCP port after Netty's {@code
bind().sync()} returns; a successful
- * TCP connect to that port therefore indicates that the worker is ready to
accept requests. The methods
- * here poll for that signal and throw {@link RuntimeException} on timeout or
if the underlying
- * {@code Process}/{@code Thread} exits before the port becomes ready.
+ * Test helpers that block until a federated worker is accepting TCP
connections on its port. The federated worker opens
+ * its TCP port after Netty's {@code bind().sync()} returns; a successful TCP
connect to that port therefore indicates
Review Comment:
While removing the html paragraph tag is good, please preserve the line
break.
##########
src/test/java/org/apache/sysds/test/component/federated/FederatedWorkerUtilsTest.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sysds.test.component.federated;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.io.IOException;
+import java.net.ServerSocket;
+
+import org.apache.sysds.test.FederatedWorkerUtils;
+import org.junit.Test;
+
+/**
+ * Tests for the readiness probe that blocks until a federated worker accepts
connections.
+ */
+public class FederatedWorkerUtilsTest {
+
+ /** Round-trip time the probe should tolerate without giving up, in ms.
*/
+ private static final int TOLERATED_HANDSHAKE_MS = 1000;
+
+ @Test
+ public void attemptBudgetCoversADelayedHandshake() {
Review Comment:
I appreciate verifying the code through these unit tests.
However, I think these tests are too fine-grained for testing the test
framework. Please remove the tests that evaluate `attemptTimeout()` directly
(aligning with removing of this method, as commented below), and move the
remaining tests to the `FederatedUrlParserTest`.
##########
src/test/java/org/apache/sysds/test/FederatedWorkerUtils.java:
##########
@@ -174,16 +180,33 @@ public static void waitForWorkers(int[] ports, int
timeoutMs, java.util.function
}
}
- private static boolean tryConnect(int port) {
+ private static boolean tryConnect(int port, long deadline) {
+ final int timeout = attemptTimeout(deadline -
System.currentTimeMillis());
+ if(timeout == 0) // out of time, do not start another attempt
Review Comment:
This condition here can be rewritten to a "less than or equal" condition,
thereby removing the necessity of the separate condition in `attemptTimeout()`
and allowing for replacing the call to `attemptTimeout()` by the single
remaining code line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
