-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 get outdated compactedTopicContext after compactionHorizon has been updated #20984
Conversation
df7ddae
to
89445e9
Compare
Codecov Report
@@ Coverage Diff @@
## master #20984 +/- ##
=============================================
+ Coverage 36.84% 73.13% +36.29%
- Complexity 12195 32257 +20062
=============================================
Files 1698 1875 +177
Lines 129852 139443 +9591
Branches 14161 15333 +1172
=============================================
+ Hits 47843 101983 +54140
+ Misses 75680 29398 -46282
- Partials 6329 8062 +1733
Flags with carried forward coverage won't be shown. Click here to find out more.
|
pulsar-broker/src/main/java/org/apache/pulsar/compaction/CompactedTopicImpl.java
Show resolved
Hide resolved
f11ba81
to
5245fa7
Compare
CompletableFuture<CompactedTopicContext> previousContext = compactedTopicContext; | ||
compactedTopicContext = openCompactedLedger(bk, compactedLedgerId); | ||
|
||
compactionHorizon = (PositionImpl) p; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can not guarantee that the reading of compactionHorizon
and compactedTopicContext
safety even if this patch is merged. We should add some code-comment on the method getCompactedTopicContextFuture
and getCompactedTopicContext
like this:
/**
* If you want to guarantee that `compactionHorizon` and `compactedTopicContext` match, you need to call this method in "synchronized(compartor){ ... }" lock block
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to give it a strong guarantee. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to give it a strong guarantee. :)
Yes, I agree. But a description is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some comments to explain this.
47ced32
to
633c1a9
Compare
633c1a9
to
8dee1bb
Compare
Motivation
In #20697, we remove the lock of get compactedTopicContext and asyncReadEntries, but
compactionHorizon
has been updated beforecompactedTopicContext
is updated. In concurrent cases, this may lead to cursor skip messages of newly compacted or getting an NPE when reading compacted entries.Modifications
Ensure
compactionHorizon
is updated aftercompactedTopicContext
.Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: