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][broker] Fix possible race condition in completing topic list watcher #18624

Merged
merged 1 commit into from
Nov 25, 2022

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Nov 25, 2022

Motivation

While reading code in TopicListService, I noticed a race condition that could happen in the case where the connection is closing and there's a TopicListWatcher that is in being initialized.

Modifications

Add check to see if the watcherFuture can be completed. If not, deregister the watcher.

Documentation

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

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 25, 2022
@lhotari
Copy link
Member Author

lhotari commented Nov 25, 2022

@andrasbeni Please review.

@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2022

Codecov Report

Merging #18624 (9f25960) into master (cd85a67) will decrease coverage by 9.87%.
The diff coverage is 9.09%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18624      +/-   ##
============================================
- Coverage     47.39%   37.52%   -9.88%     
+ Complexity    10479     7896    -2583     
============================================
  Files           698      698              
  Lines         68070    68079       +9     
  Branches       7279     7280       +1     
============================================
- Hits          32264    25547    -6717     
- Misses        32228    39217    +6989     
+ Partials       3578     3315     -263     
Flag Coverage Δ
unittests 37.52% <9.09%> (-9.88%) ⬇️

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

Impacted Files Coverage Δ
...pache/pulsar/broker/admin/v2/PersistentTopics.java 7.63% <ø> (-66.48%) ⬇️
...apache/pulsar/broker/service/TopicListService.java 56.00% <0.00%> (+1.08%) ⬆️
...pache/pulsar/client/impl/TableViewBuilderImpl.java 0.00% <0.00%> (ø)
...a/org/apache/pulsar/client/impl/TableViewImpl.java 0.00% <0.00%> (ø)
...pulsar/client/impl/TableViewConfigurationData.java 33.33% <50.00%> (+33.33%) ⬆️
...ava/org/apache/pulsar/broker/admin/v1/Brokers.java 0.00% <0.00%> (-100.00%) ⬇️
...va/org/apache/pulsar/broker/admin/v1/Clusters.java 0.00% <0.00%> (-100.00%) ⬇️
.../org/apache/pulsar/broker/admin/v1/Properties.java 0.00% <0.00%> (-100.00%) ⬇️
...ar/common/naming/PartitionedManagedLedgerInfo.java 0.00% <0.00%> (-100.00%) ⬇️
...naming/RangeEquallyDivideBundleSplitAlgorithm.java 0.00% <0.00%> (-100.00%) ⬇️
... and 163 more

@lhotari lhotari merged commit 889966c into apache:master Nov 25, 2022
@Technoboy- Technoboy- added this to the 2.12.0 milestone Nov 28, 2022
labuladong pushed a commit to labuladong/pulsar that referenced this pull request Nov 28, 2022
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Dec 9, 2022
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023
lhotari added a commit that referenced this pull request Jun 17, 2024
…tcher (#18624)

- also apply #22686 changes since that has also been cherry-picked to branch-2.11

(cherry picked from commit 889966c)
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.

4 participants