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 thread safety issue in info-internal admin api for partitioned topics #19021

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Dec 21, 2022

Motivation

  • the resulting map isn't thread safe in PartitionedManagedLedgerInfo and it's possible that the results get corrupted.

Modifications

  • it's better to refactor the logic to compose the results after the futures have completed

Documentation

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

Matching PR in forked repository

PR in forked repository: lhotari#119

…partitioned topics

- the resulting map isn't thread safe in PartitionedManagedLedgerInfo
- it's better to refactor the logic to compose the results after the futures
  have completed
@lhotari lhotari added type/bug The PR fixed a bug or issue reported a bug area/broker labels Dec 21, 2022
@lhotari lhotari added this to the 2.12.0 milestone Dec 21, 2022
@lhotari lhotari self-assigned this Dec 21, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 21, 2022
@lhotari
Copy link
Member Author

lhotari commented Dec 22, 2022

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

Codecov Report

Merging #19021 (d574cdb) into master (08591d9) will increase coverage by 0.63%.
The diff coverage is 77.77%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19021      +/-   ##
============================================
+ Coverage     49.85%   50.49%   +0.63%     
- Complexity     8658     8673      +15     
============================================
  Files           500      500              
  Lines         54930    54987      +57     
  Branches       5867     5875       +8     
============================================
+ Hits          27386    27764     +378     
+ Misses        24464    24090     -374     
- Partials       3080     3133      +53     
Flag Coverage Δ
unittests 50.49% <77.77%> (+0.63%) ⬆️

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

Impacted Files Coverage Δ
...g/apache/pulsar/broker/lookup/TopicLookupBase.java 54.02% <ø> (+3.18%) ⬆️
...rg/apache/pulsar/broker/web/PulsarWebResource.java 58.03% <ø> (+2.37%) ⬆️
...pulsar/broker/admin/impl/PersistentTopicsBase.java 59.03% <77.77%> (+0.31%) ⬆️
.../pulsar/broker/service/SharedConsumerAssignor.java 3.70% <0.00%> (-64.82%) ⬇️
...ion/buffer/metadata/TransactionBufferSnapshot.java 42.85% <0.00%> (-42.86%) ⬇️
...apache/pulsar/broker/service/EntryAndMetadata.java 0.00% <0.00%> (-40.75%) ⬇️
...saction/timeout/TransactionTimeoutTrackerImpl.java 50.87% <0.00%> (-36.85%) ⬇️
...ar/broker/loadbalance/impl/BundleSplitterTask.java 54.00% <0.00%> (-28.00%) ⬇️
...er/impl/SingleSnapshotAbortedTxnProcessorImpl.java 53.26% <0.00%> (-17.40%) ⬇️
.../apache/pulsar/broker/loadbalance/LoadManager.java 61.11% <0.00%> (-16.67%) ⬇️
... and 71 more

@Technoboy- Technoboy- merged commit 64caf49 into apache:master Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc-not-needed Your PR changes do not impact docs ready-to-test type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants