Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][client] Fix timeout handling in Pulsar Admin client #23128

Merged

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Aug 6, 2024

Fixes #23127

Motivation

In Pulsar Admin client, the read timeout is used as the request timeout. A read timeout is supposed to handle the case where the client doesn't receive any data and the connection is idle for more than the read timeout value.

Modifications

  • Use the request timeout as the request timeout instead of using the read timeout as the request timeout
  • Stop retries if the request timeout has occurred

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@lhotari lhotari self-assigned this Aug 6, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 6, 2024
@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 87.69231% with 8 lines in your changes missing coverage. Please review.

Project coverage is 74.55%. Comparing base (bbc6224) to head (f2ae67b).
Report is 812 commits behind head on master.

Files with missing lines Patch % Lines
...client/admin/internal/http/AsyncHttpConnector.java 58.82% 2 Missing and 5 partials ⚠️
...main/java/org/apache/pulsar/admin/cli/CmdBase.java 66.66% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23128      +/-   ##
============================================
+ Coverage     73.57%   74.55%   +0.98%     
- Complexity    32624    33558     +934     
============================================
  Files          1877     1919      +42     
  Lines        139502   144166    +4664     
  Branches      15299    15758     +459     
============================================
+ Hits         102638   107483    +4845     
+ Misses        28908    28448     -460     
- Partials       7956     8235     +279     
Flag Coverage Δ
inttests 27.79% <83.07%> (+3.20%) ⬆️
systests 24.76% <83.07%> (+0.44%) ⬆️
unittests 73.89% <87.69%> (+1.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...che/pulsar/client/admin/internal/BaseResource.java 78.31% <100.00%> (ø)
...ache/pulsar/client/admin/internal/BookiesImpl.java 100.00% <100.00%> (ø)
.../pulsar/client/admin/internal/BrokerStatsImpl.java 57.14% <100.00%> (ø)
...ache/pulsar/client/admin/internal/BrokersImpl.java 87.50% <100.00%> (ø)
...che/pulsar/client/admin/internal/ClustersImpl.java 97.75% <100.00%> (ø)
...ulsar/client/admin/internal/ComponentResource.java 55.00% <100.00%> (+15.00%) ⬆️
...he/pulsar/client/admin/internal/FunctionsImpl.java 44.17% <100.00%> (ø)
...e/pulsar/client/admin/internal/NamespacesImpl.java 97.67% <100.00%> (+2.22%) ⬆️
...client/admin/internal/NonPersistentTopicsImpl.java 90.90% <100.00%> (ø)
...che/pulsar/client/admin/internal/PackagesImpl.java 79.78% <100.00%> (ø)
... and 13 more

... and 483 files with indirect coverage changes

@lhotari lhotari merged commit 3b01c96 into apache:master Aug 7, 2024
51 checks passed
lhotari added a commit that referenced this pull request Aug 7, 2024
lhotari added a commit that referenced this pull request Aug 7, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 8, 2024
lhotari added a commit that referenced this pull request Aug 8, 2024
(cherry picked from commit 3b01c96)
(cherry picked from commit 0326a37)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 12, 2024
@Technoboy- Technoboy- added this to the 4.0.0 milestone Aug 14, 2024
Denovo1998 pushed a commit to Denovo1998/pulsar that referenced this pull request Aug 17, 2024
grssam pushed a commit to grssam/pulsar that referenced this pull request Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants