alex-plekhanov commented on code in PR #12710:
URL: https://github.com/apache/ignite/pull/12710#discussion_r2783239752
##########
modules/core/src/test/java/org/apache/ignite/internal/client/thin/TimeoutTest.java:
##########
@@ -217,4 +218,104 @@ 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 {
+ if (accepted == null)
Review Comment:
`accepted != null`?
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/TcpClientChannel.java:
##########
@@ -736,7 +740,7 @@ private void handshake(ProtocolVersion ver, String user,
String pwd, Map<String,
handshakeReq(ver, user, pwd, userAttrs);
try {
- ByteBuffer buf = timeout > 0 ? fut.get(timeout) : fut.get();
+ ByteBuffer buf = connTimeout > 0 ? fut.get(connTimeout) :
fut.get();
Review Comment:
We can wait here more time than handshake timeout (wait for connection
established for handshakeTimeout time, wait for each handshake request for
hanshakeTimeout time, there can be more than one hanshake request if client and
server versions are not equal). Is it expected?
##########
modules/core/src/main/java/org/apache/ignite/internal/client/thin/ClientChannelConfiguration.java:
##########
@@ -172,10 +176,26 @@ public boolean isTcpNoDelay() {
}
/**
- * @return Timeout.
+ * @deprecated Use {@link #getHandshakeTimeout()} and {@link
#getRequestTimeout()} instead.
+ * @return Request timeout.
*/
+ @Deprecated
Review Comment:
Why do we still need this method? It's in internal API and can be deleted
IMO.
##########
modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java:
##########
@@ -227,19 +231,63 @@ 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
+ ));
+ }
+
+ return reqTimeout;
}
/**
+ * @deprecated Use {@link #setHandshakeTimeout(int)} and {@link
#setRequestTimeout(int)} instead.
* @param timeout Send/receive timeout in milliseconds.
* @return {@code this} for chaining.
*/
+ @Deprecated
public ClientConfiguration setTimeout(int timeout) {
- this.timeout = timeout;
+ this.handshakeTimeout = timeout;
+ this.reqTimeout = timeout;
Review Comment:
Maybe NL after this line? (for other setters too)
--
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]
