Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Synchronous cache population could create file cache blocks with tablet ID 0 even when the remote reader had a valid tablet ID. This made per-tablet cache metadata and TTL management unable to find successfully downloaded blocks. Propagate the reader tablet ID into the cache context before creating blocks and add a unit test for the warm-up synchronous write path.
### Release note
Fix file cache blocks created by synchronous reads to retain their tablet ID.
### Check List (For Author)
- Test
- [x] Unit Test
- Added AsyncCachedRemoteFileReaderTest.sync_write_path_preserves_tablet_id.
- The modified IO library and test object compile successfully.
- Full local execution is blocked by stale local third-party Thrift/AWS SDK headers unrelated to this change.
- Behavior changed:
- [x] Yes. Synchronously populated cache blocks now retain their tablet ownership metadata.
- Does this need documentation?
- [x] No.
liaoxin01
requested review from
Gabriel39,
gavinchou and
morningman
as code owners
September 21, 2026 04:20
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![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 fresh positive-ID propagation works, but two scope/lifecycle gaps remain.
- External cached readers retain the unset
tablet_id == -1; the new unconditional sync-path assignment changes their persisted metadata namespace and feeds an invalid ID into tablet TTL maintenance. - Positive IDs survive persistence, but startup/direct restoration never repopulates the process-local TTL tablet registry, so TTL changes or expiry while the BE is down are not reconciled for fully cached tablets.
Checkpoint conclusions:
- Goal and tests: the added warm-up test proves a fresh synchronous Doris block keeps ID 10086, but it does not prove external-reader behavior or persistence/restart TTL reconciliation.
- Scope, parallel paths, and compatibility: sync, async, remote-only, peer, prefetch, existing-cell reuse, metadata direct load, and startup load were traced. The external sentinel issue and restart registration issue are the only substantiated defects; legacy ID-0 cache entries are disposable and were not treated as a separate blocker.
- Concurrency and lifecycle: no new race, lock-order, atomic, ownership, or static-initialization problem was found. The lifecycle defect is missing registration after restore, not unsafe synchronization.
- Error handling, memory, configuration, protocols, and data correctness: no new ignored status, allocation-accounting issue, configuration item, FE/BE protocol change, transaction/EditLog path, user-data visibility issue, or security boundary is involved.
- Performance and observability: the valid positive-ID assignment is negligible; invalid external TTL queue work is covered by the first inline finding. Existing logging/metrics do not correct it.
- Validation: this was a static-only review as required; I did not run builds or tests. The author reports compilation of the modified IO library/test object, while full local test execution was blocked.
- User focus: no additional review focus was provided.
Review completion: two rounds converged; both Round 2 reviewers returned NO_NEW_VALUABLE_FINDINGS. The final changed-file sweep found no unresolved candidate, and the live duplicate fence was empty on reviewed head 5d513aec8229a19d71ec1c2070b9ad680ddfc33b.
Contributor
TPC-H: Total hot run time: 27831 ms |
Contributor
TPC-DS: Total hot run time: 152661 ms |
Contributor
ClickBench: Total hot run time: 23.84 s |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
External cached readers keep the unset FileReaderOptions tablet ID of -1. Propagating that value into CacheContext would persist an invalid metadata namespace and enqueue an invalid tablet for TTL maintenance. Normalize external readers to the cache no-tablet sentinel 0, accept only positive IDs in TTL registration, and cover both synchronous and asynchronous external cache writes.
### Release note
Normalize external file cache metadata to use tablet ID 0.
### Check List (For Author)
- Test
- [x] Unit Test
- Added external reader coverage for synchronous and asynchronous cache writes.
- The modified IO library and test object compile successfully.
- Behavior changed:
- [x] No user-visible behavior change.
- Does this need documentation?
- [x] No.
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
gavinchou
approved these changes
Sep 21, 2026
Contributor
TPC-H: Total hot run time: 27754 ms |
Contributor
TPC-DS: Total hot run time: 151762 ms |
Contributor
There was a problem hiding this comment.
Request changes: fresh synchronous and external tablet-ID propagation is internally consistent, but two P2 cache-lifecycle gaps remain.
Checkpoint conclusions:
- Goal and scope: the patch correctly gives newly created synchronous Doris blocks their positive tablet ID and normalizes new external writes to 0. The remaining blockers are upgrade compatibility for persistent metadata namespaces and active-TTL reconciliation for late synchronous blocks.
- Parallel paths and compatibility: sync, async, direct/partial, remote-only, warmup/dry-run, peer, startup/direct loading, persistence, eviction, and cleanup paths were traced. Fresh paths agree; existing external -1 and synchronous Doris 0 metadata is not migrated.
- Concurrency and lifecycle: no new lock-order or ownership defect was found in fresh propagation. The migration path can mutate a live key without atomic rekeying, and TTL registration occurs before durable metadata publication without invalidating blocks_promoted.
- Tests: the new tests deterministically cover fresh sync and external sync/async identity, but not legacy-key restart orderings, eviction/restart cleanup, or late active-TTL publication.
- Configuration, API, data correctness, performance, and observability: there is no new configuration, FE/BE protocol, transaction, authoritative user-data, or independent memory/performance issue. Query correctness retains remote fallback; the findings concern persistent cache identity, TTL policy, repeated IO, and stale metadata.
- Validation: static review only, as required by the review prompt; no builds or tests were run independently. Current GitHub compile/style checks were observed, while several test/regression contexts were still pending. No additional user focus was provided.
Three review rounds converged on these two deduplicated findings; all final production, contracts/tests, and risk reviewers returned NO_NEW_VALUABLE_FINDINGS. Existing resolved threads for fresh external normalization and restore-time registration were treated as hard duplicate fences. Reviewed exact head a9b77a5 against base d93137b.
| _cache_align_mode(opts.align_mode), | ||
| _cache_write_mode(opts.cache_write_mode), | ||
| _tablet_id(opts.tablet_id), | ||
| _tablet_id(opts.is_doris_table ? opts.tablet_id : 0), |
Contributor
There was a problem hiding this comment.
[P2] Migrate legacy tablet-ID metadata keys
This normalization and the synchronous assignment below change two persistent BlockMetaKey namespaces: old async external blocks were written under -1 and now use 0, while old sync Doris blocks were written under 0 and now use the positive tablet ID. Direct loading performs only an exact (tablet_id, hash, offset) lookup, and background duplicate handling only mutates the in-memory ID; loader-first therefore retains the legacy identity, while reader-first can leave an alternate row that survives single-key eviction and is restored later for a missing file. Please reconcile this context-aware: migrate external -1 to 0, migrate legacy 0 to a positive ID only when a Doris reader supplies that identity, atomically update the live cell using the cache/block lock order, delete the old row, and cover both startup orderings through eviction and restart.
| s_align_size(offset + already_read, bytes_req - already_read, size()); | ||
| CacheContext cache_context(io_ctx); | ||
| cache_context.stats = &stats; | ||
| cache_context.tablet_id = _tablet_id; |
Contributor
There was a problem hiding this comment.
[P2] Reconcile blocks added after TTL promotion
This newly routes synchronous positive-ID blocks into the TTL manager, but register_tablet_id() only adds the ID to its deduplicated set and does not invalidate TtlInfo::blocks_promoted. SegmentIndexFileCacheLoader is a concrete caller: it forces a synchronous/dry-run INDEX read without an expiration, so after an earlier active-TTL promotion the new block is registered yet skipped by the want_ttl && blocks_promoted early return for the rest of that interval, including extensions. Registration also precedes append/finalize metadata publication, so the first scan can see no row and mark promotion complete before the block becomes durable. Please invalidate promotion at the durable publication boundary or otherwise make it level-triggered, and cover both late publication and the register-before-finalize ordering.
Contributor
ClickBench: Total hot run time: 23.97 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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:
The synchronous CachedRemoteFileReader path constructed CacheContext without copying the reader tablet ID. Cache misses were downloaded correctly, but newly created FileBlock and persisted block metadata used the no-tablet sentinel 0. Runtime per-tablet cache inspection and TTL registration therefore could not associate those new blocks with their Doris tablet.
This change propagates the Doris tablet ID into the synchronous cache context. External readers are normalized to the existing no-tablet sentinel 0 for both synchronous and asynchronous writes, and TTL registration defensively accepts only positive tablet IDs.
Release note
Fix synchronous file cache blocks to retain their Doris tablet ID while preserving no-tablet metadata for external readers.
Check List (For Author)