Fix default connectTimeout of transportBuilder #1633
Conversation
Signed-off-by: Leo <[email protected]>
Signed-off-by: Leo <[email protected]>
playLeo
requested review from
Bukhtawar,
VachaShah,
Xtansia,
dblock,
madhusudhankonda,
reta,
saratvemulapalli and
szczepanczykd
as code owners
playLeo
changed the title
Signed-off-by: Leo <[email protected]>
| private CloseableHttpAsyncClient createHttpClient() { | ||
| // default timeouts are all infinite | ||
| RequestConfig.Builder requestConfigBuilder = RequestConfig.custom() | ||
| .setConnectTimeout(Timeout.ofMilliseconds(DEFAULT_CONNECT_TIMEOUT_MILLIS)) |
There was a problem hiding this comment.
Let’s remove DEFAULT_CONNECT_TIMEOUT_MILLIS from this class since we don’t need it here now.
|
@reta Why is there a discrepancy in the default connect timeout? The default value is 1 second in ApacheHttpClient5TransportBuilder, but it’s 3 minutes by default in ConnectionConfig. |
@001mertiya This timeout setting was carried over from https://github.com/opensearch-project/OpenSearch/blob/main/client/rest/src/main/java/org/opensearch/client/RestClientBuilder.java#L71 |
Signed-off-by: Leo <[email protected]>
|
|
||
| PoolingAsyncClientConnectionManager connectionManager = PoolingAsyncClientConnectionManagerBuilder | ||
| .create() | ||
| .setDefaultConnectionConfig(connectionConfig) |
There was a problem hiding this comment.
@001mertiya could you clarify please what exactly you are trying to achieve here? The RequestConfig and ConnectionConfig are different configuration settings. If you are not satisfied with the RequestConfigsettings, use requestConfigCallback to set them to any value, including null
There was a problem hiding this comment.
@reta
The connectTimeout setting has been moved from RequestConfig to ConnectionConfig, and since the setting in RequestConfig has been deprecated, I think it should be removed. Also, the purpose of this test was to verify that the connectTimeout works correctly when a connection is established using the ConnectionConfig setting.
There was a problem hiding this comment.
Thanks @playLeo , if it was deprecated and is not working anymore, we should make sure our API does switch to ConnectionConfig from RequestConfig
There was a problem hiding this comment.
Thank you, my point is: we have class RequestConfigCallback [1] that historically was used to configure timeouts in question. Now, since has timeout related methods deprecated, we could (and probably should) reflect that by introducing ConnectionConfigCallback to allow configuring these timeouts.
There was a problem hiding this comment.
Thank you for the detailed explanation. I understand. I have a question before I start working. It seems that you can configure RequestConfig and ConnectionConfig either by using HttpClientConfigCallback or by using RequestConfigCallback and ConnectionConfigCallback. Doesn’t having both ways to set these configurations create potential confusion? What is the benefit of using ConnectionConfigCallback?
There was a problem hiding this comment.
Thanks @playLeo , since the timeout properties from RequestConfig have been moved (and potentially removed), the ConnectionConfig is the way to configure those (we would need to document that).
There was a problem hiding this comment.
we could (and probably should) reflect that by introducing
ConnectionConfigCallbackto allow configuring these timeouts.
I think I did not explain it clearly. If we create a ConnectionConfigCallback we would then have two ways to set the ConnectionConfig one through the existing HttpClientConfigCallback and one through the new ConnectionConfigCallback so I am wondering if the ConnectionConfigCallback is really necessary.
I also know that for RequestConfig there are two ways to configure it using HttpClientConfigCallback and RequestConfigCallback but I am wondering if the RequestConfigCallback is really needed in that case too.
If there are two ways to configure this, wouldn’t that be confusing? Are you saying it’s fine as long as it’s documented properly?
There was a problem hiding this comment.
If there are two ways to configure this, wouldn’t that be confusing? Are you saying it’s fine as long as it’s documented properly?
Yeah, so the ConnectionConfigCallback is only useful when the caller does not want to deal with connection manager (since it is part of it). In this case, providing just ConnectionConfigCallback and not HttpClientConfigCallback would work just fine. Thanks.
There was a problem hiding this comment.
Please take a look at the updated
playLeo
force-pushed
the
fix/connectTimeout
branch
from
5b8654e to
26a3872
Compare
Signed-off-by: Leo <[email protected]>
playLeo
force-pushed
the
fix/connectTimeout
branch
from
26a3872 to
aff280f
Compare
|
Hi @playLeo, can you pull the latest changes from main and add these test on top of that? There was an issue with socketTimeout as well, fixed in the latest main. |
3 participants
Description
ConnectionConfig connectTimeout is used only when the connectTimeout in RequestConfig is null.
However, the builder is currently setting a value for connectTimeout in RequestConfig, which causes the value in ConnectionConfig to be ignored.
Therefore, it is safe to remove the default setting in RequestConfig.
Also, ConnectionConfig already defines a default value:
Timeout DEFAULT_CONNECT_TIMEOUT = Timeout.ofMinutes(3)Issues Resolved
This PR is related to #1581
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.