Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[improve][ml] RangeCache refactoring: test race conditions and prevent endless loops #22814

Merged
merged 8 commits into from
May 31, 2024

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented May 31, 2024

Motivation

Follow up on #22789. Test a few basic race conditions.
The solution in #22789 could lead to an endless loop. This PR fixes this problem.

Race conditions are logged with info level, for example:

2024-05-31T08:57:44,823 - INFO  - [pool-2-thread-4:RangeCache@308] - Value was already released for key 59, removing entry without releasing the value
2024-05-31T08:57:44,824 - INFO  - [pool-2-thread-4:RangeCache@308] - Value was already released for key 153, removing entry without releasing the value
2024-05-31T08:57:44,824 - INFO  - [pool-2-thread-7:RangeCache@308] - Value was already released for key 153, removing entry without releasing the value
2024-05-31T08:57:44,824 - INFO  - [pool-2-thread-8:RangeCache@308] - Value was already released for key 60, removing entry without releasing the value
2024-05-31T08:57:44,827 - INFO  - [pool-2-thread-2:RangeCache@296] - Key 580 does not match the entry's value wrapper's key 660, removing entry by key without releasing the value
2024-05-31T08:57:44,827 - INFO  - [pool-2-thread-3:RangeCache@296] - Key 226 does not match the entry's value wrapper's key 668, removing entry by key without releasing the value
2024-05-31T08:57:44,827 - INFO  - [pool-2-thread-4:RangeCache@308] - Value was already released for key 741, removing entry without releasing the value
2024-05-31T08:57:44,829 - INFO  - [pool-2-thread-6:RangeCache@308] - Value was already released for key 980, removing entry without releasing the value
2024-05-31T08:57:44,830 - INFO  - [pool-2-thread-6:RangeCache@308] - Value was already released for key 1074, removing entry without releasing the value
2024-05-31T08:57:44,830 - INFO  - [pool-2-thread-3:RangeCache@296] - Key 1088 does not match the entry's value wrapper's key 1358, removing entry by key without releasing the value
2024-05-31T08:57:44,911 - INFO  - [main:RangeCache@337] - Value RangeCacheTest.RefString(s=1, matchingKey=123) does not match the key 1, removed entry without releasing the value
2024-05-31T08:57:44,912 - INFO  - [main:RangeCache@308] - Value was already released for key 1, removing entry without releasing the value

Modifications

  • test some invalid entry cases which could be a result of race conditions
  • prevent endless loops in evictLeastAccessedEntries, evictLEntriesBeforeTimestamp and clear.
    • invalid entries cannot be skipped since .firstEntry() method is used for iterating in removal. If the entry isn't removed and persists, it causes the endless loop.
  • add logic for skipping invalid entries for removeRange

Documentation

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

@lhotari lhotari requested a review from merlimat May 31, 2024 06:39
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 31, 2024
@lhotari lhotari force-pushed the lh-rangecache-followup branch from 7309311 to 28cf786 Compare May 31, 2024 06:44
@lhotari lhotari force-pushed the lh-rangecache-followup branch from 28cf786 to 9ca1777 Compare May 31, 2024 06:58
- prevent getting the value of a different key if the wrapper has been recycled
@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 73.25%. Comparing base (bbc6224) to head (bb1689c).
Report is 322 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22814      +/-   ##
============================================
- Coverage     73.57%   73.25%   -0.33%     
+ Complexity    32624     2511   -30113     
============================================
  Files          1877     1889      +12     
  Lines        139502   141692    +2190     
  Branches      15299    15553     +254     
============================================
+ Hits         102638   103792    +1154     
- Misses        28908    29877     +969     
- Partials       7956     8023      +67     
Flag Coverage Δ
inttests 27.36% <62.50%> (+2.77%) ⬆️
systests 24.76% <53.12%> (+0.44%) ⬆️
unittests 72.28% <78.12%> (-0.57%) ⬇️

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

Files Coverage Δ
...org/apache/bookkeeper/mledger/util/RangeCache.java 85.36% <78.12%> (-10.04%) ⬇️

... and 389 files with indirect coverage changes

@lhotari lhotari merged commit e731674 into apache:master May 31, 2024
50 checks passed
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
…t endless loops (apache#22814)

(cherry picked from commit e731674)
(cherry picked from commit 1f04038)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2024
…t endless loops (apache#22814)

(cherry picked from commit e731674)
(cherry picked from commit 1f04038)
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants