Skip to content

fix(segment): close removed PinotDataBuffer in FilePerIndexDirectory#18405

Merged
Jackie-Jiang merged 3 commits intoapache:masterfrom
dkranchii:fix/file-per-index-directory-buffer-leak
May 7, 2026
Merged

fix(segment): close removed PinotDataBuffer in FilePerIndexDirectory#18405
Jackie-Jiang merged 3 commits intoapache:masterfrom
dkranchii:fix/file-per-index-directory-buffer-leak

Conversation

@dkranchii
Copy link
Copy Markdown
Contributor

@dkranchii dkranchii commented May 3, 2026

FilePerIndexDirectory.removeIndex() was dropping the buffer entry
from the cache map without calling close() on it. For mmap-backed
buffers (the default for offline segments), this means the kernel
mapping leaks for the lifetime of the JVM — the on-disk file does
get deleted, but the address-space mapping stays until the process
exits (or the Cleaner happens to fire, which we shouldn't rely on
for deterministic cleanup).

removeIndex() is hit during segment reloads, minion-driven segment
conversions, and the various *IndexHandler drop/replace flows, so on
long-running servers that churn through reloads this shows up as the
classic.

The class itself had a // TODO flagging this, so this PR just
finishes it.

Scope

Single file: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectory.java
~10 lines changed

No public API change, no behavior change for callers and removeIndex still returns void. It still cleans up the on-disk files exactly the same way, just additionally closes the cached buffer.

Testing

Existing FilePerIndexDirectoryTest still passes.
Manually verified the flow: with the fix, the PinotDataBuffer.close() on the removed entry is invoked once; without the fix it never was.

Fixes #18404

Release note

none (memory leak fix, no user-visible behavior change)

@dkranchii dkranchii changed the title fix(segment): close removed PinotDataBuffer in FilePerIndexDirectory.… fix(segment): close removed PinotDataBuffer in FilePerIndexDirectory May 3, 2026
@dkranchii
Copy link
Copy Markdown
Contributor Author

@xiangfu0 can you please review this pr. thanks

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 4, 2026

Codecov Report

❌ Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.60%. Comparing base (80fb0e8) to head (44afbfa).
⚠️ Report is 19 commits behind head on master.

Files with missing lines Patch % Lines
...ent/local/segment/store/FilePerIndexDirectory.java 70.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18405      +/-   ##
============================================
+ Coverage     63.46%   63.60%   +0.14%     
- Complexity     1701     1717      +16     
============================================
  Files          3254     3252       -2     
  Lines        199104   199141      +37     
  Branches      30830    30876      +46     
============================================
+ Hits         126354   126668     +314     
+ Misses        62665    62400     -265     
+ Partials      10085    10073      -12     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-21 63.60% <70.00%> (+0.14%) ⬆️
temurin 63.60% <70.00%> (+0.14%) ⬆️
unittests 63.60% <70.00%> (+0.14%) ⬆️
unittests1 55.68% <10.00%> (+0.26%) ⬆️
unittests2 34.91% <70.00%> (-0.07%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Jackie-Jiang Jackie-Jiang added bug Something is not working as expected ingestion Related to data ingestion pipeline labels May 6, 2026
Co-authored-by: Xiang Fu <xiangfu.1024@gmail.com>
@dkranchii dkranchii force-pushed the fix/file-per-index-directory-buffer-leak branch from ad46be0 to 44afbfa Compare May 6, 2026 19:49
@dkranchii
Copy link
Copy Markdown
Contributor Author

@Jackie-Jiang updated the pr. can you please review. thanks

@Jackie-Jiang Jackie-Jiang merged commit 4863cdc into apache:master May 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something is not working as expected ingestion Related to data ingestion pipeline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Off-heap mmap buffer leaked in FilePerIndexDirectory.removeIndex()

4 participants