[fix] [ml] The atomicity of multiple fields of ml is broken #19346
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
codelipenghui
approved these changes
Jan 29, 2023
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
Outdated
Show resolved
Hide resolved
|
As #16420 cherry-picked into the previous branches, I've added labels too. |
labuladong
reviewed
Jan 29, 2023
Comment on lines
1557
to
1563
| synchronized (ManagedLedgerImpl.this) { | ||
| ledgers.put(lh.getId(), newLedger); | ||
| currentLedger = lh; | ||
| currentLedgerEntries = 0; | ||
| currentLedgerSize = 0; | ||
| updateLedgersIdsComplete(); | ||
| mbean.addLedgerSwitchLatencySample(System.currentTimeMillis() |
There was a problem hiding this comment.
Is it necessary to move updateLedgersIdsComplete() into the synchronized block? I see before #16420 this method is out of synchronized too.
There was a problem hiding this comment.
Yes, it is not necessary. But it should be executed after these fields changes.
labuladong
approved these changes
Jan 29, 2023
Codecov Report
@@ Coverage Diff @@
## master #19346 +/- ##
=============================================
+ Coverage 32.45% 64.01% +31.56%
- Complexity 6348 25871 +19523
=============================================
Files 1644 1818 +174
Lines 123737 133112 +9375
Branches 13494 14644 +1150
=============================================
+ Hits 40162 85218 +45056
+ Misses 77654 40085 -37569
- Partials 5921 7809 +1888
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Technoboy-
approved these changes
Jan 29, 2023
Technoboy-
pushed a commit
that referenced
this pull request
Feb 8, 2023
liangyepianzhou
pushed a commit
that referenced
this pull request
Feb 9, 2023
(cherry picked from commit e2851da)
nicoloboschi
pushed a commit
to datastax/pulsar
that referenced
this pull request
Feb 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
ML uses synchronized to guarantee the atomicity of modifications to variables
ledgers,currentLedger,currentLedgerEntriesandcurrentLedgerSize, but PR #16420 breaks atomicity, resulting in incorrect results when using those variables(E.g calculate backlog size by special position)https://github.com/apache/pulsar/pull/16420/files#diff-f6a849bd8fdb782ef6c17a2e07a2c54c3dc7d1655c00ec3546d5f3b3fc61e970L1485-R1499
Modifications
Make changes to these variables within the synchronized block.
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: