timoninmaxim commented on code in PR #12569:
URL: https://github.com/apache/ignite/pull/12569#discussion_r2648388045
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ClientChannelConfiguration.java:
##########
@@ -172,10 +176,26 @@ public boolean isTcpNoDelay() {
}
/**
+ * @deprecated Use {@link #getConnectionTimeout()} and {@link
#getRequestTimeout()} instead.
* @return Timeout.
Review Comment:
Request timeout
##########
modules/core/src/test/java/org/apache/ignite/internal/client/thin/TimeoutTest.java:
##########
@@ -217,4 +217,120 @@ public void testClientTimeoutOnOperation() throws
Exception {
}
}
}
+
+ /**
+ * Test that connection timeout is independent of request timeout during
connection establishment.
+ */
+ @Test
+ @SuppressWarnings("ThrowableNotThrown")
+ public void testConnectionTimeoutIndependentOfRequest() throws Exception {
+ ServerSocket sock = new ServerSocket();
+ sock.bind(new InetSocketAddress("127.0.0.1", DFLT_PORT));
+
+ CountDownLatch connectionAccepted = new CountDownLatch(1);
+
+ IgniteInternalFuture<?> fut = GridTestUtils.runAsync(() -> {
+ try {
+ Socket accepted = sock.accept();
+ connectionAccepted.countDown();
+
+ Thread.sleep(2000);
+
+ U.closeQuiet(accepted);
+ }
+ catch (Exception e) {
+ throw new IgniteException("Accept thread failed: " +
e.getMessage(), e);
+ }
+ });
+
+ long startTime = System.currentTimeMillis();
+
+ try {
+ ClientConfiguration cfg = new ClientConfiguration()
+ .setAddresses("127.0.0.1:" + DFLT_PORT)
+ .setConnectionTimeout(500)
+ .setRequestTimeout(10000);
+
+ GridTestUtils.assertThrowsWithCause(
+ () -> Ignition.startClient(cfg),
+ ClientConnectionException.class
+ );
+ }
+ finally {
+ U.closeQuiet(sock);
+ }
+
+ long elapsed = System.currentTimeMillis() - startTime;
+
+ assertTrue("Should fail around connection timeout (500ms), not request
timeout",
+ elapsed >= 450 && elapsed < 2000);
+
+ assertTrue("Connection should have been accepted",
connectionAccepted.await(1, TimeUnit.SECONDS));
+
+ fut.get();
+ }
+
+ /**
+ * Test that request timeout is independent of connection timeout during
operations.
+ */
+ @Test
+ @SuppressWarnings("ThrowableNotThrown")
+ public void testRequestTimeoutIndependentOfConnection() throws Exception {
Review Comment:
Similar for this test:
1. Set server and connection timeous to Integer.MAX_VALUE
2. Just check that exception contains timeout exception
(X.hasCause(IgniteFutureTimeoutCheckedException.class))
##########
modules/core/src/test/java/org/apache/ignite/internal/client/thin/TimeoutTest.java:
##########
@@ -217,4 +217,120 @@ public void testClientTimeoutOnOperation() throws
Exception {
}
}
}
+
+ /**
+ * Test that connection timeout is independent of request timeout during
connection establishment.
+ */
+ @Test
+ @SuppressWarnings("ThrowableNotThrown")
+ public void testConnectionTimeoutIndependentOfRequest() throws Exception {
+ ServerSocket sock = new ServerSocket();
+ sock.bind(new InetSocketAddress("127.0.0.1", DFLT_PORT));
+
+ CountDownLatch connectionAccepted = new CountDownLatch(1);
+
+ IgniteInternalFuture<?> fut = GridTestUtils.runAsync(() -> {
+ try {
+ Socket accepted = sock.accept();
+ connectionAccepted.countDown();
+
+ Thread.sleep(2000);
+
+ U.closeQuiet(accepted);
+ }
+ catch (Exception e) {
+ throw new IgniteException("Accept thread failed: " +
e.getMessage(), e);
+ }
+ });
+
+ long startTime = System.currentTimeMillis();
+
+ try {
+ ClientConfiguration cfg = new ClientConfiguration()
+ .setAddresses("127.0.0.1:" + DFLT_PORT)
+ .setConnectionTimeout(500)
+ .setRequestTimeout(10000);
+
+ GridTestUtils.assertThrowsWithCause(
+ () -> Ignition.startClient(cfg),
+ ClientConnectionException.class
+ );
+ }
+ finally {
+ U.closeQuiet(sock);
+ }
+
+ long elapsed = System.currentTimeMillis() - startTime;
+
+ assertTrue("Should fail around connection timeout (500ms), not request
timeout",
Review Comment:
This checks are not stable and must be avoided:
1. System.currentTimeMillis() returns the wall-clock time, which is
non-monotonic and susceptible to adjustments, such as NTP synchronization or
manual system clock updates
2. Thread might be blocked more than 2s.
Just set request timeout to Integer.MAX_VALUE. Also server handshake time
must be overridden - see `getConfiguration` in this class.
##########
modules/core/src/test/java/org/apache/ignite/internal/client/thin/TimeoutTest.java:
##########
@@ -217,4 +217,120 @@ public void testClientTimeoutOnOperation() throws
Exception {
}
}
}
+
+ /**
+ * Test that connection timeout is independent of request timeout during
connection establishment.
+ */
+ @Test
+ @SuppressWarnings("ThrowableNotThrown")
+ public void testConnectionTimeoutIndependentOfRequest() throws Exception {
+ ServerSocket sock = new ServerSocket();
+ sock.bind(new InetSocketAddress("127.0.0.1", DFLT_PORT));
+
+ CountDownLatch connectionAccepted = new CountDownLatch(1);
+
+ IgniteInternalFuture<?> fut = GridTestUtils.runAsync(() -> {
+ try {
+ Socket accepted = sock.accept();
+ connectionAccepted.countDown();
+
+ Thread.sleep(2000);
+
+ U.closeQuiet(accepted);
+ }
+ catch (Exception e) {
+ throw new IgniteException("Accept thread failed: " +
e.getMessage(), e);
+ }
+ });
+
+ long startTime = System.currentTimeMillis();
+
+ try {
+ ClientConfiguration cfg = new ClientConfiguration()
+ .setAddresses("127.0.0.1:" + DFLT_PORT)
+ .setConnectionTimeout(500)
+ .setRequestTimeout(10000);
+
+ GridTestUtils.assertThrowsWithCause(
Review Comment:
Let's check that a timeout exception is mentioned in this exception message
##########
modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java:
##########
@@ -227,19 +230,55 @@ public ClientConfiguration setTcpNoDelay(boolean
tcpNoDelay) {
}
/**
+ * @deprecated Use {@link #getConnectionTimeout()} and {@link
#getRequestTimeout()} instead.
* @return Send/receive timeout in milliseconds.
Review Comment:
Request timeout
##########
modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java:
##########
@@ -227,19 +230,55 @@ public ClientConfiguration setTcpNoDelay(boolean
tcpNoDelay) {
}
/**
+ * @deprecated Use {@link #getConnectionTimeout()} and {@link
#getRequestTimeout()} instead.
* @return Send/receive timeout in milliseconds.
*/
+ @Deprecated
public int getTimeout() {
- return timeout;
+ return reqTimeout;
Review Comment:
Let's add WARN log if `reqTimeout != connTimeout`. The warn message should
mention that deprecated API is used and request timeout is returned.
--
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]
