Copilot commented on code in PR #12710:
URL: https://github.com/apache/ignite/pull/12710#discussion_r2787314417
##########
modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java:
##########
@@ -57,8 +58,11 @@ public final class ClientConfiguration implements
Serializable {
/** @serial Tcp no delay. */
private boolean tcpNoDelay = true;
- /** @serial Timeout. 0 means infinite. */
- private int timeout;
+ /** @serial Handshake timeout in milliseconds. 0 means infinite. */
+ private int handshakeTimeout;
+
+ /** @serial Request timeout in milliseconds. 0 means infinite. */
+ private int reqTimeout;
Review Comment:
ClientConfiguration is Serializable with a fixed serialVersionUID (0L), but
the legacy `timeout` field has been removed and replaced with
`handshakeTimeout`/`reqTimeout`. Deserializing previously-serialized
ClientConfiguration instances will now silently lose the timeout value (new
fields default to 0). Consider preserving serialization compatibility by
keeping a deprecated `timeout` field and/or implementing custom deserialization
(e.g., mapping legacy `timeout` to both new fields when present).
##########
modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java:
##########
@@ -227,18 +231,65 @@ public ClientConfiguration setTcpNoDelay(boolean
tcpNoDelay) {
}
/**
- * @return Send/receive timeout in milliseconds.
+ * @deprecated Use {@link #getHandshakeTimeout()} and {@link
#getRequestTimeout()} instead.
+ * @return Request timeout in milliseconds.
*/
+ @Deprecated
public int getTimeout() {
- return timeout;
+ if (reqTimeout != handshakeTimeout) {
+ U.warn(logger, String.format(
+ "Deprecated getTimeout() API is used while request timeout
(%d) differs from handshake timeout (%d). " +
+ "Returning request timeout. Please use getRequestTimeout()
and getHandshakeTimeout() instead.",
+ reqTimeout, handshakeTimeout
+ ));
+ }
Review Comment:
`getTimeout()` now logs a warning whenever request/handshake timeouts
differ. Since this is a getter, callers may invoke it frequently (including via
legacy code paths), which can spam logs. Consider logging this warning only
once (e.g., via a static/instance guard) or moving the warning to the setters
when a divergence is introduced.
##########
modules/core/src/test/java/org/apache/ignite/internal/client/thin/TimeoutTest.java:
##########
@@ -217,4 +218,103 @@ 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));
+
+ IgniteInternalFuture<?> fut = GridTestUtils.runAsync(() -> {
+ Socket accepted = null;
+
+ try {
+ accepted = sock.accept();
+
+ while (!Thread.currentThread().isInterrupted())
+ Thread.sleep(10);
+ }
+ finally {
+ U.closeQuiet(accepted);
+ }
+ });
+
+ try {
+ ClientConfiguration cfg = new ClientConfiguration()
+ .setAddresses("127.0.0.1:" + DFLT_PORT)
+ .setHandshakeTimeout(500)
+ .setRequestTimeout(Integer.MAX_VALUE);
+
+ GridTestUtils.assertThrowsWithCause(
+ () -> Ignition.startClient(cfg),
+ IgniteFutureTimeoutCheckedException.class
+ );
+
+ fut.cancel();
+ }
+ finally {
Review Comment:
In `testConnectionTimeoutIndependentOfRequest`, the accept thread loops
until interrupted, but `fut.cancel()` is only called after the assertion
succeeds. If `assertThrowsWithCause` fails (or throws unexpectedly), the test
can leave a non-terminating background thread running. Move `fut.cancel()` (and
ideally `fut.get()`/await) into the `finally` block to guarantee cleanup even
on assertion failures.
```suggestion
}
finally {
fut.cancel();
```
--
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]
