IGNITE-20973 Unusable thin client timeout #12569
Conversation
modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java
Show resolved
Hide resolved
VolkodamovDaniil
force-pushed
the
ignite-20973
branch
from
996bd45 to
a051e32
Compare
| /** | ||
| * @return Connection timeout in milliseconds. 0 means infinite. | ||
| */ | ||
| public int getConnTimeout() { |
There was a problem hiding this comment.
Do not use abbreviations for method names
modules/core/src/main/java/org/apache/ignite/configuration/ClientConfiguration.java
Show resolved
Hide resolved
| @Deprecated | ||
| public int getTimeout() { | ||
| return timeout; | ||
| return Math.max(connTimeout, reqTimeout); |
There was a problem hiding this comment.
Let's return req timeout here
| ClientConfiguration target = new ClientConfiguration() | ||
| .setAddresses("127.0.0.1:10800", "127.0.0.1:10801") | ||
| .setTimeout(123) | ||
| .setConnectionTimeout(123) |
There was a problem hiding this comment.
Let's add a test with different timeouts for connection and requests
| /** @serial Connection timeout in milliseconds. 0 means infinite. */ | ||
| private int connTimeout; | ||
|
|
||
| /** @serial Request timeout in milliseconds. 0 means infinite. */ |
There was a problem hiding this comment.
Please check that actual javadocs contains lines started with 'serial'. For which purpose this tag is used?
|
|
||
| /** | ||
| * @deprecated Use {@link #getConnectionTimeout()} and {@link #getRequestTimeout()} instead. | ||
| * @return Timeout. |
| .setConnectionTimeout(500) | ||
| .setRequestTimeout(10000); | ||
|
|
||
| GridTestUtils.assertThrowsWithCause( |
There was a problem hiding this comment.
Let's check that a timeout exception is mentioned in this exception message
|
|
||
| long elapsed = System.currentTimeMillis() - startTime; | ||
|
|
||
| assertTrue("Should fail around connection timeout (500ms), not request timeout", |
There was a problem hiding this comment.
This checks are not stable and must be avoided:
- System.currentTimeMillis() returns the wall-clock time, which is non-monotonic and susceptible to adjustments, such as NTP synchronization or manual system clock updates
- 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.
| */ | ||
| @Test | ||
| @SuppressWarnings("ThrowableNotThrown") | ||
| public void testRequestTimeoutIndependentOfConnection() throws Exception { |
There was a problem hiding this comment.
Similar for this test:
- Set server and connection timeous to Integer.MAX_VALUE
- Just check that exception contains timeout exception (X.hasCause(IgniteFutureTimeoutCheckedException.class))
| @Deprecated | ||
| public int getTimeout() { | ||
| return timeout; | ||
| return reqTimeout; |
There was a problem hiding this comment.
Let's add WARN log if reqTimeout != connTimeout. The warn message should mention that deprecated API is used and request timeout is returned.
|
|
||
| /** | ||
| * @deprecated Use {@link #getConnectionTimeout()} and {@link #getRequestTimeout()} instead. | ||
| * @return Send/receive timeout in milliseconds. |
3 participants
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.