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][ml] Fix race conditions in RangeCache #22789

Merged
merged 7 commits into from
May 31, 2024

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented May 28, 2024

Motivation

The RangeCache class contains several race conditions which cause instability.

When one thread removes the entry and another one uses it, that will become a problem.

The cacheEvictionIntervalMs setting is 10 ms by default. This results in the
RangeCache.evictLEntriesBeforeTimestamp method getting called about 100 times per second.
The default expiration is managedLedgerCacheEvictionTimeThresholdMillis which is 1000 ms by default.

It's also possible that 2 threads remove the entry at the same time. ManagedLedgerImpl.invalidateEntriesUpToSlowestReaderPosition will result in calls to RangeCache.removeRange method. These calls happen independently of the RangeCache.evictLEntriesBeforeTimestamp calls so there's a chance for race conditions.

Modifications

  • Add checks to validate that the value hasn't already been released before it is removed.
    • add extra .retain() and .release() calls
    • add a way to validate that the value matches the key
      • this is to prevent a possible race condition where the entry has already been recycled and contains another valid entry which is for a different key
  • Make all removals using the remove(key, value) method so that removals remove the correct value exactly once
    • Add a wrapper object for the value to ensure this so that exact identity match is used for the removal.
    • The wrapper instance is recycled to reduce the overhead.

Documentation

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

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2024

Codecov Report

Attention: Patch coverage is 86.45833% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 73.27%. Comparing base (bbc6224) to head (2a6c2b1).
Report is 321 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22789      +/-   ##
============================================
- Coverage     73.57%   73.27%   -0.31%     
- Complexity    32624    32648      +24     
============================================
  Files          1877     1889      +12     
  Lines        139502   141659    +2157     
  Branches      15299    15543     +244     
============================================
+ Hits         102638   103800    +1162     
- Misses        28908    29844     +936     
- Partials       7956     8015      +59     
Flag Coverage Δ
inttests 27.46% <76.04%> (+2.88%) ⬆️
systests 24.82% <72.91%> (+0.50%) ⬆️
unittests 72.27% <86.45%> (-0.58%) ⬇️

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

Files Coverage Δ
.../org/apache/bookkeeper/mledger/impl/EntryImpl.java 78.31% <0.00%> (-0.17%) ⬇️
...org/apache/bookkeeper/mledger/util/RangeCache.java 89.31% <87.36%> (-6.09%) ⬇️

... and 367 files with indirect coverage changes

@lhotari lhotari self-assigned this May 30, 2024
@lhotari lhotari marked this pull request as draft May 30, 2024 20:05
@lhotari lhotari marked this pull request as ready for review May 30, 2024 21:06
@lhotari
Copy link
Member Author

lhotari commented May 30, 2024

Previous work in RangeCache to prevent race conditions: #15707 and #21302.

@lhotari lhotari requested a review from Technoboy- May 30, 2024 22:02
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

Very nice work

@lhotari
Copy link
Member Author

lhotari commented May 31, 2024

This PR contained a few issues. I have a follow up PR #22814 to address the issues. Please review

lhotari added a commit that referenced this pull request Jun 3, 2024
lhotari added a commit that referenced this pull request Jun 3, 2024
lhotari added a commit that referenced this pull request Jun 3, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2024
(cherry picked from commit c39f9f8)
(cherry picked from commit 9a99e45)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2024
(cherry picked from commit c39f9f8)
(cherry picked from commit 9a99e45)
@lhotari lhotari added this to the 4.0.0 milestone Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants