Skip to content

Conversation

@lhotari
Copy link
Member

@lhotari lhotari commented Sep 28, 2022

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)

  • 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: lhotari#93

@lhotari
Copy link
Member Author

lhotari commented Sep 28, 2022

I noticed this while working on #17252

Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link

github-actions bot commented Nov 3, 2022

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

@github-actions github-actions bot added the Stale label Nov 3, 2022
…urrentLedgerSize fields

- currentLedgerEntries and currentLedgerSize fields are currently mutated and
  read from multiple threads which is a problem.
  - one solution is to make the fields volatile
@lhotari lhotari force-pushed the lh-fix-ledger-counters-threadsafety branch from 1b413f1 to fdff41a Compare December 20, 2022 18:47
@michaeljmarshall michaeljmarshall added type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use area/broker labels Dec 20, 2022
@lhotari lhotari added this to the 2.12.0 milestone Dec 20, 2022
@lhotari lhotari self-assigned this Dec 20, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.48%. Comparing base (22866bd) to head (fdff41a).
Report is 1933 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #17868      +/-   ##
============================================
+ Coverage     45.92%   46.48%   +0.55%     
- Complexity    10104    10477     +373     
============================================
  Files           680      706      +26     
  Lines         66758    69004    +2246     
  Branches       7147     7392     +245     
============================================
+ Hits          30660    32077    +1417     
- Misses        32680    33323     +643     
- Partials       3418     3604     +186     
Flag Coverage Δ
unittests 46.48% <100.00%> (+0.55%) ⬆️

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

Files with missing lines Coverage Δ
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java 53.81% <100.00%> (+0.04%) ⬆️

... and 69 files with indirect coverage changes

@lhotari lhotari merged commit ef0f385 into apache:master Dec 20, 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 Stale type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants