pulsar icon indicating copy to clipboard operation
pulsar copied to clipboard

[fix][broker] Fix thread-safety issue with currentLedgerEntries and currentLedgerSize fields

Open lhotari opened this issue 3 years ago • 2 comments

Motivation

  • currentLedgerEntries and currentLedgerSize fields in the ManagedLedgerImpl class are currently mutated and read from multiple threads which is a problem.
    • one solution is to make the fields volatile

Modifications

  • make fields volatile

Documentation

  • [ ] doc-required (Your PR needs to update docs and you will update later)

  • [x] doc-not-needed (Please explain why)

  • [ ] doc (Your PR contains doc changes)

  • [ ] doc-complete (Docs have been already added)

Matching PR in forked repository

PR in forked repository: https://github.com/lhotari/pulsar/pull/93

lhotari avatar Sep 28 '22 09:09 lhotari

I noticed this while working on #17252

lhotari avatar Sep 28 '22 09:09 lhotari

The pr had no activity for 30 days, mark with Stale label.

github-actions[bot] avatar Nov 03 '22 02:11 github-actions[bot]

Codecov Report

Merging #17868 (fdff41a) into master (22866bd) will decrease coverage by 10.02%. The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #17868       +/-   ##
=============================================
- Coverage     45.92%   35.90%   -10.03%     
+ Complexity    10104     7754     -2350     
=============================================
  Files           680      706       +26     
  Lines         66758    69004     +2246     
  Branches       7147     7392      +245     
=============================================
- Hits          30660    24776     -5884     
- Misses        32680    40894     +8214     
+ Partials       3418     3334       -84     
Flag Coverage Δ
unittests 35.90% <100.00%> (-10.03%) :arrow_down:

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

Impacted Files Coverage Δ
...va/org/apache/pulsar/broker/service/ServerCnx.java 48.19% <ø> (-2.26%) :arrow_down:
.../java/org/apache/pulsar/client/impl/ClientCnx.java 29.97% <ø> (ø)
...org/apache/pulsar/proxy/server/ProxyClientCnx.java 46.51% <ø> (ø)
...rg/apache/pulsar/proxy/server/ProxyConnection.java 55.21% <ø> (ø)
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java 53.81% <100.00%> (+0.04%) :arrow_up:
...ava/org/apache/pulsar/broker/admin/v1/Brokers.java 0.00% <0.00%> (-100.00%) :arrow_down:
...va/org/apache/pulsar/broker/admin/v1/Clusters.java 0.00% <0.00%> (-100.00%) :arrow_down:
.../org/apache/pulsar/broker/admin/v1/Properties.java 0.00% <0.00%> (-100.00%) :arrow_down:
.../apache/pulsar/broker/admin/v2/ResourceGroups.java 0.00% <0.00%> (-100.00%) :arrow_down:
...ar/common/naming/PartitionedManagedLedgerInfo.java 0.00% <0.00%> (-100.00%) :arrow_down:
... and 158 more

codecov-commenter avatar Dec 20 '22 19:12 codecov-commenter