[fix](cloud) bind the packed slice location lifetime to its writer - #67347
Open
liaoxin01 wants to merge 3 commits into
Open
[fix](cloud) bind the packed slice location lifetime to its writer#67347liaoxin01 wants to merge 3 commits into
liaoxin01 wants to merge 3 commits into
Conversation
liaoxin01
requested review from
Gabriel39,
gavinchou and
luwei16
as code owners
August 31, 2026 11:00
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
liaoxin01
force-pushed
the
fix-packed-slice-index-retention
branch
from
September 3, 2026 08:41
ae1340a to
7545454
Compare
liaoxin01
changed the title
liaoxin01
force-pushed
the
fix-packed-slice-index-retention
branch
from
September 3, 2026 08:57
7545454 to
9ab63d9
Compare
liaoxin01
changed the title
liaoxin01
force-pushed
the
fix-packed-slice-index-retention
branch
from
September 3, 2026 11:41
9ab63d9 to
668245e
Compare
A small file is handed to `PackedFileManager` when the segment is flushed (`close(true)` / `InvertedIndexFileWriter::begin_close()`), but its slice location is read again much later, when the rowset is finally closed: `wait_upload_done()` from `finish_close()`, and `get_packed_slice_location()` when the rowset meta is built. The gap between the two is the duration of the whole load, not the lifetime of the packed file. Both reads went through `_global_slice_locations`, which the background cleanup recycles purely on the age of the small file (`uploaded_file_retention_seconds`, 1800s by default). Any load running longer than that lost the mapping and failed with: [INTERNAL_ERROR]File not found in global index: data/<tablet_id>/<segment>_0.idx The packed file itself had been uploaded successfully, only the in-memory mapping was gone. In a case we hit in production a broker load ran for 38 minutes; the index file was packed and uploaded in the first minute and the rowset was closed in the 38th. `_uploaded_packed_files` is recycled on the same config, so a load could also fail later with "Packed file not found for path". Hand the slice location out as a `std::shared_ptr` instead of looking it up by path: - `append_small_file()` returns the location, and `PackedFileWriter` keeps it for as long as it lives. `wait_upload_done()` and the writer's `get_packed_slice_location()` take that reference, so neither can fail because the index was recycled, and `CloudRowsetWriter` collects the location from the writer rather than from the manager. - The background cleanup is unchanged: recycling an index entry can no longer invalidate a writer, it only drops the by-path lookup that readers use. - The terminal upload state of a packed file is recorded on the slices it contains, so a waiter no longer needs the `PackedFileContext` either. The contexts keep their existing retention. This also removes a hazard around a path being written twice within one process (a MOW partial update creates a transient rowset writer per publish attempt, each with the same rowset id and the same first segment id, hence the same segment path): the two writes now own separate locations, where before the second one replaced the first in the index and both writers read whichever was last written. A null handle means append_small_file() skipped the data without writing it anywhere, which appendv() normally prevents by switching to direct write first. `small_file_threshold_bytes` is mutable at runtime though, so lowering it between the last append and close reaches that path. Report it instead of closing successfully on a file that does not exist.
liaoxin01
force-pushed
the
fix-packed-slice-index-retention
branch
from
September 8, 2026 04:11
668245e to
f882b77
Compare
Contributor
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
liaoxin01
requested review from
airborne12,
csun5285,
eldenmoon and
yiguolei
as code owners
September 8, 2026 13:53
liaoxin01
force-pushed
the
fix-packed-slice-index-retention
branch
from
September 8, 2026 14:51
a51fd83 to
d43901d
Compare
… held `cleanup_expired_data()` recycles `_global_slice_locations` purely on the age of the small file. Age says nothing about whether the entry is still needed: a load can run for much longer than `uploaded_file_retention_seconds`, and everything it wrote stays readable through the index until the rowset is committed. `PackedFileSystem::open_file_impl()` falls back to that index for a file with no slice location in the rowset meta yet, which is every read that happens during the load. `SegmentIndexFileCacheLoader` does exactly that: it reads a segment back right after close, to preload its index and footer ranges into file cache. It works today only because that read follows the close within seconds. The previous commit made the slice location a `shared_ptr` held by the writer, so the index can now tell whether anyone still needs an entry: skip the ones some writer or packed file context still holds, and let the age check apply only to those the index alone holds. The last owner going away drops the reference for us, so the entry's lifetime follows its readers instead of the wall clock. `use_count()` is only a snapshot, but every transition here is a decrement -- a handle gains owners in `append_small_file()` before it is ever visible to the cleanup. Losing a race just keeps an entry for one more round. `WaitUploadDoneSurvivesRecycledIndexAndContext` asserted the old semantics: it kept a handle and a context alive across the cleanup and then required the index to be empty, which is exactly what this change stops from happening. Split it in two -- one case for a recycled context with the writer still open, one that removes the index entry outright to show the handle stands on its own.
`PackedFileContext::slice_locations` is keyed by the small file path, so writing one path twice into the same packed file leaves only the second handle in there. `mark_slices_upload_result()` iterated that map, so the shadowed slice never reached a terminal state: it stayed PENDING with `packed_file_size` at -1, and once the context was recycled its writer's `wait_upload_done()` failed with `Packed file not found`. Keep every appended handle alongside the by-path map and notify from that. The map still describes what goes to the meta service, where a duplicate path cannot be represented anyway. No caller does this today -- `CloudTablet::create_transient_rowset_writer()` sets `allow_packed_file = false`, so the MOW publish retry that writes one segment path twice never produces packed files. The existing rewrite test rotates the packed file between the two appends, so it never covered a duplicate within one packed file either.
liaoxin01
force-pushed
the
fix-packed-slice-index-retention
branch
from
September 8, 2026 15:23
d43901d to
a9927c3
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16976 ms |
Contributor
TPC-DS: Total hot run time: 82340 ms |
Contributor
ClickBench: Total hot run time: 14.76 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
A small file is handed to
PackedFileManagerwhen the segment is flushed, but its slicelocation is read again only when the rowset is closed —
wait_upload_done()fromfinish_close(), andget_packed_slice_location()when the rowset meta is built. The gapbetween the two is the duration of the whole load, not the lifetime of the packed file.
Both reads went through
_global_slice_locations, whichcleanup_expired_data()recyclespurely on the age of the small file (
uploaded_file_retention_seconds, 1800s by default).Any load running longer than that lost the mapping and failed with:
The packed file itself had been uploaded successfully — only the in-memory mapping was gone.
We hit this in production with a broker load that ran for 38 minutes. Two related points:
_uploaded_packed_filesis recycled on the same config, so a waiter could also fail withPacked file not found for path: ...; and this is not specific to inverted index files,since segments smaller than
small_file_threshold_bytestake the same async-close /late-wait path.
What is changed and how it works?
A slice location should live as long as whoever may still read it, not until a wall clock
deadline.
append_small_file()returns the location as astd::shared_ptrandPackedFileWriterkeeps it for as long as it lives.
wait_upload_done()and the writer'sget_packed_slice_location()take that reference, andCloudRowsetWriterasks the writerinstead of the manager, so neither can fail because the index was recycled. The terminal
upload state (UPLOADED / FAILED) is recorded on the slices, so a waiter does not need the
PackedFileContexteither.The index itself gets the same property.
PackedFileSystem::open_file_impl()falls backto
_global_slice_locationsfor a file whose location has not reached the rowset metayet, which is every read during the load —
SegmentIndexFileCacheLoaderdoes exactly thatwhen it preloads a segment's index and footer ranges into file cache right after close.
Since a location is now a
shared_ptr, the cleanup can tell whether an entry is stillneeded: entries some writer or packed file context still holds are skipped, and the age
check applies only to those the index alone holds. The last owner going away drops the
reference, so an entry outlives everything that could still read it.
PackedFileContext::slice_locationsis keyed by the small file path, so one path writtentwice into the same packed file left only the second handle in there and the first never
reached a terminal state. Every appended handle is now kept alongside that map and
notified from it. No caller does this today —
CloudTablet::create_transient_rowset_writer()setsallow_packed_file = false, so theMOW publish retry that writes one segment path twice never produces packed files — but
the interface allowed it and failed quietly.
Behavior note: an index entry is no longer guaranteed to be dropped after
uploaded_file_retention_seconds. It now stays for as long as a writer or a packed filecontext holds the slice, so a load producing N packed small files keeps N entries resident
until it finishes. That is the point of the fix — those entries were being dropped while
still in use — but it does change how much the map holds during a long load.
Behavior for short loads is unchanged.
Release note
Fix
File not found in global indexfailures for loads that run longer thanuploaded_file_retention_secondswhen packed file is enabled.Check List (For Author)
Test
Behavior changed:
Does this need documentation?