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][admin] Fix exception loss in getMessageId method #23766

Merged
merged 1 commit into from
Dec 21, 2024

Conversation

danpi
Copy link
Contributor

@danpi danpi commented Dec 20, 2024

Fixes #23765

Motivation

#23765 demonstrated how to reproduce the TimeoutException bug and explained the potential issue. Therefore, this PR aims to fix this problem.

Modifications

  1. The exception is returned via completeExceptionally instead of being thrown directly.
  2. A new test case is added to testGetMessageById to verify the effect of the fix.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added test in
    org/apache/pulsar/broker/admin/PersistentTopicsTest.java#testGetMessageById

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
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

Copy link

@danpi Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@github-actions github-actions bot added doc-label-missing doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Dec 20, 2024
@danpi
Copy link
Contributor Author

danpi commented Dec 20, 2024

The reason behind this is that there is a possibility of calling the asyncReadEntry0 method in RangeEntryCacheImpl, where at line 272, a thread switch occurs, causing the exception to be lost. However, in the case of an asynchronous call, if we cannot predict when the thread switch will happen, returning the exception via completeExceptionally instead of throwing it directly might be a better approach.

@danpi danpi marked this pull request as draft December 20, 2024 10:19
@danpi danpi marked this pull request as ready for review December 20, 2024 10:20
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, good catch

@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

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

Files with missing lines Patch % Lines
...pulsar/broker/admin/impl/PersistentTopicsBase.java 66.66% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23766      +/-   ##
============================================
+ Coverage     73.57%   74.30%   +0.73%     
+ Complexity    32624    32125     -499     
============================================
  Files          1877     1838      -39     
  Lines        139502   143054    +3552     
  Branches      15299    16232     +933     
============================================
+ Hits         102638   106299    +3661     
+ Misses        28908    28385     -523     
- Partials       7956     8370     +414     
Flag Coverage Δ
inttests 26.71% <0.00%> (+2.13%) ⬆️
systests 23.67% <0.00%> (-0.66%) ⬇️
unittests 73.69% <66.66%> (+0.84%) ⬆️

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

Files with missing lines Coverage Δ
...pulsar/broker/admin/impl/PersistentTopicsBase.java 69.80% <66.66%> (+4.35%) ⬆️

... and 1008 files with indirect coverage changes

@lhotari lhotari merged commit 3d50574 into apache:master Dec 21, 2024
57 of 58 checks passed
lhotari pushed a commit that referenced this pull request Dec 21, 2024
lhotari pushed a commit that referenced this pull request Dec 21, 2024
lhotari pushed a commit that referenced this pull request Dec 21, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 26, 2024
Co-authored-by: houbonan <[email protected]>
(cherry picked from commit 3d50574)
(cherry picked from commit 79e05a2)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 26, 2024
Co-authored-by: houbonan <[email protected]>
(cherry picked from commit 3d50574)
(cherry picked from commit 79e05a2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] TimeoutException encountered while accessing the admin's getMessageById API in version 3.0.7
4 participants