Skip to content
New issue

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

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

Already on GitHub? # to your account

[improve][admin] Print error log if handle http response fails #23563

Merged

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Nov 5, 2024

Motivation

When users are calling pulsar-admin API, and there is an internal error( such as a deserialize JSON error), there is no error log.

You can test it this way:

  • add a method getMsgRateIn() for TopicStatsImpl, and return a String value(such as abc), which will make the response.msgRateIn as a String value abc.
  • call pulsar-admin topics stats
    • You will get nothing, and no error log is printed.

Modifications

Print error log if handle HTTP response fails

Documentation

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

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode added this to the 4.1.0 milestone Nov 5, 2024
@poorbarcode poorbarcode self-assigned this Nov 5, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 5, 2024
@lhotari lhotari changed the title [improve] [log] Print error log if handle http response fails [improve][admin] Print error log if handle http response fails Nov 5, 2024
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.33%. Comparing base (bbc6224) to head (368653b).
Report is 714 commits behind head on master.

Files with missing lines Patch % Lines
...client/admin/internal/http/AsyncHttpConnector.java 50.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23563      +/-   ##
============================================
+ Coverage     73.57%   74.33%   +0.75%     
- Complexity    32624    34327    +1703     
============================================
  Files          1877     1943      +66     
  Lines        139502   147056    +7554     
  Branches      15299    16210     +911     
============================================
+ Hits         102638   109310    +6672     
- Misses        28908    29327     +419     
- Partials       7956     8419     +463     
Flag Coverage Δ
inttests 27.65% <50.00%> (+3.07%) ⬆️
systests 24.37% <50.00%> (+0.04%) ⬆️
unittests 73.72% <50.00%> (+0.87%) ⬆️

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

Files with missing lines Coverage Δ
...client/admin/internal/http/AsyncHttpConnector.java 85.15% <50.00%> (-1.27%) ⬇️

... and 655 files with indirect coverage changes

@Technoboy- Technoboy- merged commit c15a0d6 into apache:master Nov 6, 2024
59 of 62 checks passed
visxu pushed a commit to vissxu/pulsar that referenced this pull request Nov 6, 2024
poorbarcode added a commit that referenced this pull request Nov 12, 2024
poorbarcode added a commit that referenced this pull request Nov 12, 2024
poorbarcode added a commit that referenced this pull request Nov 12, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 13, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 13, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants