You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "oneby-wang (via GitHub)" <gi...@apache.org> on 2025/12/05 07:47:06 UTC
[PR] [fix][admin] Use ClientConfigurationData timeout params to set AsyncHttpConnector timeout params [pulsar]
oneby-wang opened a new pull request, #25040:
URL: https://github.com/apache/pulsar/pull/25040
### Motivation
In previous code, we use `client.getConfiguration().getProperty(ClientProperties.CONNECT_TIMEOUT)` as PulsarAdmin's `connectionTimeoutMs`, use `client.getConfiguration().getProperty(ClientProperties.READ_TIMEOUT)` as PulsarAdmin's `readTimeoutMs`, use `PulsarAdminImpl.DEFAULT_REQUEST_TIMEOUT_SECONDS * 1000` as PulsarAdmin's `requestTimeoutMs`. See:
https://github.com/apache/pulsar/blob/b71bea4c42353f9d14f817aa9a0877d495336f34/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java#L121-L128
Before this PR, we can't tune `requestTimeoutMs`, which is always 30000s = 5min. See also in:
https://github.com/apache/pulsar/issues/24879#issuecomment-3440341666
### Modifications
Now, we use `ClientConfigurationData.requestTimeoutMs` as `AsyncHttpConnector.requestTimeoutMs` , so we can control the http retry timeout by tuning `ClientConfigurationData.requestTimeoutMs`.
AsyncHttpClient get Connector by calling `ConnectorProvider#getConnector(Client client, Configuration runtimeConfig)` method.
https://github.com/eclipse-ee4j/jersey/blob/bb8a2a1b78e39604f4d283d0176705b143355cce/core-client/src/main/java/org/glassfish/jersey/client/ClientConfig.java#L465-L467
Both `connectionTimeoutMs` and `readTimeoutMs` are the same values in jersey properties and `ClientConfigurationData`. See:
https://github.com/apache/pulsar/blob/b71bea4c42353f9d14f817aa9a0877d495336f34/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/PulsarAdminImpl.java#L146-L150
To ensure code consistency, I would like to suggest using `ClientConfigurationData` and removing jersey config.
Side effect: default `requestTimeoutMs` changes from 30s to 60s after this PR, which I think is reasonable. If I didn't read the source code, I would assume `requestTimeoutMs` is set to 60s, and it would confuse me to find that `requestTimeoutMs` didn't take effect after I adjusted this value.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: https://github.com/oneby-wang/pulsar/pull/11
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][admin] Use ClientConfigurationData timeout params to set AsyncHttpConnector timeout params [pulsar]
Posted by "oneby-wang (via GitHub)" <gi...@apache.org>.
oneby-wang commented on code in PR #25040:
URL: https://github.com/apache/pulsar/pull/25040#discussion_r2592559354
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java:
##########
@@ -216,7 +210,6 @@ private void configureAsyncHttpClientConfig(ClientConfigurationData conf, int co
confBuilder.setCookieStore(null);
confBuilder.setUseProxyProperties(true);
confBuilder.setFollowRedirect(false);
- confBuilder.setRequestTimeout(conf.getRequestTimeoutMs());
Review Comment:
The `requestTimeoutMs` param is set twice in this method: once via `confBuilder.setRequestTimeout(conf.getRequestTimeoutMs())` and once via `confBuilder.setReadTimeout(readTimeoutMs)`. For this reason, I removed the line `confBuilder.setRequestTimeout(conf.getRequestTimeoutMs())`.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][admin] Use ClientConfigurationData timeout params to set AsyncHttpConnector timeout params [pulsar]
Posted by "oneby-wang (via GitHub)" <gi...@apache.org>.
oneby-wang commented on code in PR #25040:
URL: https://github.com/apache/pulsar/pull/25040#discussion_r2592555118
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java:
##########
Review Comment:
@lhotari hidden here
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][admin] Use ClientConfigurationData timeout params to set AsyncHttpConnector timeout params [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat merged PR #25040:
URL: https://github.com/apache/pulsar/pull/25040
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][admin] Use ClientConfigurationData timeout params to set AsyncHttpConnector timeout params [pulsar]
Posted by "oneby-wang (via GitHub)" <gi...@apache.org>.
oneby-wang commented on code in PR #25040:
URL: https://github.com/apache/pulsar/pull/25040#discussion_r2592555118
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java:
##########
Review Comment:
@lhotari here
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java:
##########
@@ -216,7 +210,6 @@ private void configureAsyncHttpClientConfig(ClientConfigurationData conf, int co
confBuilder.setCookieStore(null);
confBuilder.setUseProxyProperties(true);
confBuilder.setFollowRedirect(false);
- confBuilder.setRequestTimeout(conf.getRequestTimeoutMs());
Review Comment:
The `requestTimeoutMs` param is set twice in this method: once via `confBuilder.setRequestTimeout(conf.getRequestTimeoutMs())` and once via `confBuilder.setReadTimeout(readTimeoutMs)`. For this reason, I removed the line `confBuilder.setRequestTimeout(conf.getRequestTimeoutMs())`.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][admin] Use ClientConfigurationData timeout params to set AsyncHttpConnector timeout params [pulsar]
Posted by "oneby-wang (via GitHub)" <gi...@apache.org>.
oneby-wang commented on code in PR #25040:
URL: https://github.com/apache/pulsar/pull/25040#discussion_r2592571679
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java:
##########
@@ -216,7 +210,6 @@ private void configureAsyncHttpClientConfig(ClientConfigurationData conf, int co
confBuilder.setCookieStore(null);
confBuilder.setUseProxyProperties(true);
confBuilder.setFollowRedirect(false);
- confBuilder.setRequestTimeout(conf.getRequestTimeoutMs());
Review Comment:
The `requestTimeoutMs` param is set twice in this method: once via `confBuilder.setRequestTimeout(conf.getRequestTimeoutMs())` and once via `confBuilder.setReadTimeout(readTimeoutMs)`. For this reason, I removed the line `confBuilder.setRequestTimeout(conf.getRequestTimeoutMs())`.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org