[fix](be) remove changing segment cache blocks to index type#65905
Open
bobhan1 wants to merge 2 commits into
Open
[fix](be) remove changing segment cache blocks to index type#65905bobhan1 wants to merge 2 commits into
bobhan1 wants to merge 2 commits into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Add a deterministic end-to-end SegmentWriter and S3 multipart BE unit test that reproduces cache corruption when multipart writes complete out of order. The test lets the S3 buffer at offset 256 fill an unaligned cache block [241, 304] before the buffer at offset 0. Both direct cache reads and cached S3 reads then return bytes shifted by 15 positions. This commit intentionally keeps the failing assertions to record the existing problem before the fix.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ./run-be-ut.sh --run --filter=SegmentWriterFileCacheConcurrencyTest.ConcurrentLaterPartMustNotShiftCachedSegmentBytes -j100 (expected failure reproducing shifted bytes in cache block [241, 304])
- Behavior changed: No
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: SegmentWriter finalized a cloud segment by allocating a cache holder over the segment index range and changing every intersecting block to INDEX. The range can start inside an S3 multipart buffer, so the newly allocated cache block can cross a multipart boundary. If a later multipart write completes first, it can claim that block and populate its beginning with bytes from the later buffer, shifting the cached segment bytes. Remove the post-finalize holder allocation and cache-type change. Also remove the temporary reproduction test and synchronization hooks; the preceding commit retains the deterministic failing reproducer in the PR history.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ./run-be-ut.sh --run --filter=CloudFileCacheWriteIndexOnlyTest.* -j100
- The preceding commit reproducer fails as expected with shifted bytes in cache block [241, 304]
- Behavior changed: Yes. Segment cache blocks keep the cache type selected by the file writer instead of being changed to INDEX after segment finalization.
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
bobhan1
marked this pull request as ready for review
July 22, 2026 10:40
bobhan1
requested review from
airborne12,
csun5285,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
July 22, 2026 10:40
bobhan1
changed the title
Contributor
Author
|
run buildall |
12 tasks
gavinchou
approved these changes
Jul 22, 2026
Contributor
Author
|
/review |
bobhan1
added a commit
to bobhan1/doris
that referenced
this pull request
Jul 22, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#65905 Problem Summary: SegmentWriter::finalize allocated a cache holder over the segment index range and changed every intersecting block to INDEX. The range can start inside a file-cache block and conflict with canonical async-read probe ranges or cross S3 multipart write boundaries. The previous workaround aligned all FileCacheAllocatorBuilder allocations and added a SegmentWriter alignment test, but that changes the behavior of every writer-side allocation to compensate for a holder whose only purpose is cache-type reclassification. Follow the narrower solution from apache#65905: remove the post-finalize holder allocation and change_cache_type call. Segment cache blocks now retain the type selected by the file writer. Remove the allocator-alignment workaround, its accessor and comments, and the temporary synchronization hook and end-to-end alignment test. Keep the strict probe contract and the independent file-tail and downloader-ownership fixes. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=CloudFileCacheWriteIndexOnlyTest.* -j100 (3 tests passed) - build-support/check-format.sh - Behavior changed: Yes. Segment cache blocks are no longer reclassified to INDEX after SegmentWriter finalization. - Does this need documentation: No
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
TPC-H: Total hot run time: 29655 ms |
Contributor
TPC-DS: Total hot run time: 176600 ms |
bobhan1
added a commit
to bobhan1/doris
that referenced
this pull request
Jul 22, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#65905 Problem Summary: SegmentWriter::finalize allocated a cache holder over the segment index range and changed every intersecting block to INDEX. The range can start inside a file-cache block and conflict with canonical async-read probe ranges or cross S3 multipart write boundaries. The previous workaround aligned all FileCacheAllocatorBuilder allocations and added a SegmentWriter alignment test, but that changes the behavior of every writer-side allocation to compensate for a holder whose only purpose is cache-type reclassification. Follow the narrower solution from apache#65905: remove the post-finalize holder allocation and change_cache_type call. Segment cache blocks now retain the type selected by the file writer. Remove the allocator-alignment workaround, its accessor and comments, and the temporary synchronization hook and end-to-end alignment test. Keep the strict probe contract and the independent file-tail and downloader-ownership fixes. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=CloudFileCacheWriteIndexOnlyTest.* -j100 (3 tests passed) - build-support/check-format.sh - Behavior changed: Yes. Segment cache blocks are no longer reclassified to INDEX after SegmentWriter finalization. - Does this need documentation: No
Contributor
ClickBench: Total hot run time: 24.92 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes. The deletion removes the demonstrated finalize-time overlapping holder, but runtime part-size mutation leaves a residual unaligned-holder corruption path. It also removes the existing INDEX-queue policy for ordinary non-TTL SegmentWriter cache writes, and the final tree does not retain a regression test for the repaired ordering.
Critical checkpoint conclusions:
- Goal and data correctness: the finalize-time holder is removed, but the runtime-mutable part-size issue shows that a misaligned dynamic update lets the remaining holder recreate the byte shift. Remote object bytes remain correct; the affected surface is the local cache copy.
- Concurrency and lifecycle: the relevant actors are the finalize/non-block-close thread, S3 upload-pool workers, published cache readers, and local cache block ownership. Remote close is not a local-cache-copy barrier when async cache flush is enabled. Part/block alignment must be a lifetime/per-writer invariant, and any replacement classification needs a separate cache-copy barrier or atomic promotion.
- Scope, parallel paths, and compatibility: no protocol, format, transaction, MoW, version, or persistence change is involved. TTL and index-only loader modes are unaffected; vertical/packed paths did not use this conversion. The ordinary non-TTL SegmentWriter path has the cache-residency regression described inline.
- Tests: retained index-only tests do not reach the removed cache-builder branch or compare cache bytes with the remote object. A final deterministic regression is required.
- Performance and observability: removing the oversized allocation is beneficial, but losing the separately-sized INDEX queue and its longer hot-data interval can force avoidable remote metadata reads. Existing cache/preload metrics are otherwise sufficient.
- Error handling, memory safety, and configuration: deletion removes an ignored
change_cache_type()Status and adds no allocation or lock hazard. The existing mutable part-size configuration lacks update validation/per-writer snapshotting and leaves a correctness hole. - User focus: no extra focus points were provided; the full one-file change and its upstream/downstream cache paths were reviewed.
Validation was static as required by the review runner; no local builds or source modifications were performed.
| LOG(INFO) << "segment flush consumes a lot time_ns " << timer.elapsed_time() | ||
| << ", segmemt_size " << *segment_file_size; | ||
| } | ||
| // When the cache type is not ttl(expiration time == 0), the data should be split into normal cache queue |
Contributor
There was a problem hiding this comment.
The retained test suite does not exercise this fix. CloudFileCacheWriteIndexOnlyTest forces index-only mode, which gives segment writers no cache builder and mocks the loader before the actual cache read, so the deleted branch was unreachable even before this patch. The deterministic out-of-order test from 028891cdc38 is removed by the head commit, meaning this data-corruption path can be reintroduced while CI stays green. Please retain an adapted passing test that blocks adjacent mock multipart uploads, lets finalize() return, releases the later remote upload and waits until its cache block is DOWNLOADED, then releases the earlier upload, waits for both local copies and close, and compares cached bytes—including the cached-reader path—with the assembled remote object.
| } | ||
| // When the cache type is not ttl(expiration time == 0), the data should be split into normal cache queue | ||
| // and index cache queue | ||
| if (auto* cache_builder = _file_writer->cache_builder(); cache_builder != nullptr && |
Contributor
There was a problem hiding this comment.
This deletion removes the finalize-time unaligned holder, but it does not make the remaining S3 part holders unconditionally aligned. s3_write_buffer_size is runtime-mutable, while divisibility by immutable file_cache_each_block_size is checked only during ExecEnv::init_file_cache_factory(). For example, updating 5 MiB to 5.5 MiB before a writer makes part 2 start at 5.5 MiB. Because async cache flush signals remote completion before its local copy, a published reader can first install the aligned 5-6 MiB block; part 2 then intersects it and copies buffer position zero at 5 MiB, shifting cached bytes by 0.5 MiB—the same corruption class this PR targets. Please make the part size immutable, or validate aligned updates and snapshot one validated size per S3FileWriter; include this contract in the retained regression.
| auto size = *index_size + *segment_file_size; | ||
| auto holder = cache_builder->allocate_cache_holder(index_start, size, _tablet->tablet_id()); | ||
| for (auto& segment : holder->file_blocks) { | ||
| static_cast<void>(segment->change_cache_type(io::FileCacheType::INDEX)); |
Contributor
There was a problem hiding this comment.
The corruption comes from allocating an overlapping holder while multipart cache copies are active, but deleting the whole block also removes the only INDEX classification for ordinary non-TTL SegmentWriter cache writes. FileCacheAllocatorBuilder creates those blocks as NORMAL, later is_index_data reads only touch the block in its existing queue, and SegmentIndexFileCacheLoader runs only under enable_file_cache_write_index_file_only (default false). Thus explicit/adaptive full-cache writes leave segment metadata in the NORMAL queue, with a 1-day hot-data interval, instead of the separately-sized INDEX queue with a 7-day interval; normal data churn can now evict metadata and force remote reads. Please preserve the policy without recreating an unaligned live holder—for example, wait on a separate local cache-copy completion barrier before promoting only recorded ranges, or atomically promote existing blocks on index access. SegmentFileCollection::close() alone is not such a barrier when enable_flush_file_cache_async is true.
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
3 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?
Issue Number: None
Related PR: None
Problem Summary:
After a cloud segment is finalized,
SegmentWriterallocates a cache holder over the segment index range and changes every intersecting cache block toINDEX. This extra holder is not aligned to S3 multipart buffer boundaries. In the deterministic reproduction, it creates cache block[241, 304], which crosses the boundary between buffers starting at offsets 0 and 256.Non-blocking close allows those multipart buffers to finish out of order. When the buffer at offset 256 finishes first, it claims
[241, 304]and writes its bytes at the beginning of that block. The cached data is therefore shifted by 15 bytes, while the multipart object in remote storage remains correct. Both a direct cache read and a cached S3 read observe the corrupted bytes.Remove the post-finalize cache-holder allocation and
change_cache_type(INDEX)behavior. Segment blocks keep the cache type selected by the file writer, and this cross-buffer cache block is no longer created bySegmentWriter.The two commits intentionally preserve the proof in history:
028891cdc38adds a deterministic end-to-end BEUT and fails on the existing behavior.0d6964d2f80removes the behavior and removes the temporary BEUT and synchronization hooks.The final PR diff contains only the production-code deletion.
Release note
None
Check List (For Author)
./run-be-ut.sh --run --filter=SegmentWriterFileCacheConcurrencyTest.ConcurrentLaterPartMustNotShiftCachedSegmentBytes -j100on the first commit: expected failure, reproducing shifted cached bytes in[241, 304]./run-be-ut.sh --run --filter=CloudFileCacheWriteIndexOnlyTest.* -j100on the final commit: 3 tests passedINDEXafter segment finalization.