pulsar icon indicating copy to clipboard operation
pulsar copied to clipboard
verified publisher icon apache

[fix][admin] Use ClientConfigurationData timeout params to set AsyncHttpConnector timeout params

Open oneby-wang opened this issue 1 month ago • 0 comments

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:

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

  • [ ] doc
  • [ ] doc-required
  • [x] doc-not-needed
  • [ ] doc-complete

Matching PR in forked repository

PR in forked repository: https://github.com/oneby-wang/pulsar/pull/11

oneby-wang avatar Dec 05 '25 07:12 oneby-wang