Skip to content

[fix][ml] PIP-430: Fix concurrency issue in MessageMetadata caching and improve caching #24836

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.

Sign up for GitHub

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

Jump to bottom
Merged
lhotari merged 5 commits into from
Oct 10, 2025
Merged

[fix][ml] PIP-430: Fix concurrency issue in MessageMetadata caching and improve caching #24836

lhotari merged 5 commits into from
Oct 10, 2025

Conversation

@lhotari
Copy link
Member

@lhotari lhotari commented Oct 10, 2025

Fix #24831

Motivation

The test ManagedCursorConcurrencyTest.testConcurrentReadOfSameEntry is flaky. In that case, the entries don't contain any message metadata. The broker cache will attempt to parse the message metadata each time the entry is cached. This results in a race condition since peekMessageMetadata changes the readerIndex of the cached ByteBuf while another thread is making a retained duplicate of the cached ByteBuf. Thanks for @pdolif for the analysis of the problem. It was helpful in locating the issue.

Modifications

  • increase invocation count and concurrency in the ManagedCursorConcurrencyTest.testConcurrentReadOfSameEntry test to reproduce the issue more often
  • eliminate race conditions in org.apache.bookkeeper.mledger.impl.EntryImpl#initializeMessageMetadataIfNeeded
  • eliminate race conditions in org.apache.bookkeeper.mledger.impl.cache.RangeCacheEntryWrapper#getValueInternal
  • copy MessageMetadata from the entry that is inserted into the cache. This optimization was missing.

Documentation

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

@lhotari lhotari added this to the 4.2.0 milestone Oct 10, 2025
@lhotari lhotari self-assigned this Oct 10, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 10, 2025
@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2025

Codecov Report

❌ Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.28%. Comparing base (cc5e479) to head (8545f44).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...per/mledger/impl/cache/RangeCacheEntryWrapper.java 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #24836       +/-   ##
=============================================
+ Coverage     38.36%   74.28%   +35.91%     
+ Complexity    13123     2494    -10629     
=============================================
  Files          1856     1913       +57     
  Lines        145070   149236     +4166     
  Branches      16836    17320      +484     
=============================================
+ Hits          55662   110861    +55199     
+ Misses        81843    29530    -52313     
- Partials       7565     8845     +1280
Flag Coverage Δ
inttests 26.29% <87.50%> (-0.08%) ⬇️
systests 22.76% <87.50%> (-0.01%) ⬇️
unittests 73.81% <87.50%> (+39.27%) ⬆️

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

Files with missing lines Coverage Δ
.../org/apache/bookkeeper/mledger/impl/EntryImpl.java 90.34% <100.00%> (+40.34%) ⬆️
...keeper/mledger/impl/cache/RangeEntryCacheImpl.java 71.06% <100.00%> (+19.22%) ⬆️
...per/mledger/impl/cache/RangeCacheEntryWrapper.java 88.05% <75.00%> (+3.21%) ⬆️

... and 1416 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lhotari lhotari merged commit 9737d03 into apache:master Oct 10, 2025
100 of 103 checks passed
lhotari added a commit that referenced this pull request Oct 10, 2025
@lhotari
[fix][ml] PIP-430: Fix concurrency issue in MessageMetadata caching a…
5aebfed 
…nd improve caching (#24836)

(cherry picked from commit 9737d03)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dao-jun dao-jun dao-jun approved these changes

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

@nodece nodece Awaiting requested review from nodece

+1 more reviewer

@pdolif pdolif pdolif approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lhotari lhotari

Labels

area/ML cherry-picked/branch-4.1 doc-not-needed Your PR changes do not impact docs ready-to-test release/4.1.2 type/bug The PR fixed a bug or issue reported a bug type/flaky-tests

Projects

None yet

Milestone

4.2.0

Development

Successfully merging this pull request may close these issues.

Flaky-test: ManagedCursorConcurrencyTest.testConcurrentReadOfSameEntry

4 participants

@lhotari @codecov-commenter @dao-jun @pdolif