Repository navigation
[fix](be) Restore file-level cache metadata loading during startup - #69116
Merged
Merged
Conversation
Related PR: apache#57072, apache#65658 Problem Summary: During BE startup, get_or_set, the fully-covered lookup, and probe attempt to restore an absent file before background metadata loading completes. Their FileCacheKey leaves offset uninitialized, while the storage implementation now performs an exact RocksDB lookup for one block. A file with multiple persisted blocks can therefore miss existing cache data or restore only one block. Query metadata by tablet and file hash prefix, restore all blocks, and assert that the file hash is absent when entering synchronous restoration. Keep caller trigger conditions, key encoding, and existing tablet-range queries unchanged. Fix reuse of persistent file cache blocks while BE startup metadata loading is in progress. - Test: Unit Test via run-be-ut.sh with ASAN and GLIBC_COMPATIBILITY=OFF: 38 related tests passed; 9 focused tests passed again after final cleanup. The 5 startup regression cases failed before the fix and passed afterward. clang-format v16, build hygiene, git diff checks, and clang-tidy on changed lines passed. - Behavior changed: Yes, startup lookups restore all persisted blocks for an absent file instead of querying one uninitialized offset. - Does this need documentation: No
deardeng
requested review from
Gabriel39,
gavinchou,
liaoxin01 and
morningman
as code owners
October 15, 2026 06:19
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of all 10 changed files at f8f8121 found no new actionable defects. The review converged in round 1: both complete-review agents and the separate startup-concurrency reviewer returned NO_NEW_VALUABLE_FINDINGS, and all initial risks were independently checked and disposed of. There are no existing inline threads or blocking comments to carry forward. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and evidence: all three absent-file startup lookups now restore the tablet/hash record range rather than one uninitialized offset. The added tests cover five paused-loader lookup cases, completed-startup gating, and file-prefix isolation. Their code and expectations were inspected; tests were not executed.
- Scope and reuse: the changes are focused; shared prefix serialization serves full keys and both range-query variants.
- Concurrency and locks: request lookups and background insertion serialize map/LRU/size updates with the cache mutex. Background merges retain duplicate handling. The absent-hash assertion follows from every caller's locked lookup. No new lock-order issue was found; the full scan and insertion remain inside the existing lock.
- Lifecycle and static initialization: iterators own their copied prefix and are scoped within DB ownership. Loader/writer shutdown joins and drains existing work. Test callbacks resume before cache destruction. No new static initialization dependency or ownership cycle was found.
- Configuration: no changes; dynamic propagation is not applicable.
- Compatibility: persisted key bytes and value format remain unchanged, including high-bit hash fields. Internal interface declarations, implementations and callers are updated consistently; no new wire or rolling-upgrade incompatibility was identified.
- Thrift/protobuf definitions: none changed; field-ID branch checks do not apply.
- Parallel paths: get_or_set, fully-covered lookup and probe are covered; memory storage remains a no-op, and tablet-only TTL queries retain their behavior.
- Conditions and errors: new missing-DB/malformed-record branches explain their concrete failure cases and match existing optional-cache recovery. The absent-file invariant is checked at restoration entry.
- Test coverage: nonzero offsets, supported unaligned requests, multiple blocks, original metadata, missing/adjacent prefixes and completed-startup gating are covered by the added test code. These metadata-only startup tests do not read backing files or assert accounting after background loading resumes; runtime TTL, corruption, failure and latency behavior was not exercised.
- Expected results: no golden result files were added or changed; inspected unit-test expectations match the intended metadata behavior.
- Observability: existing lookup/probe timing and warning paths cover restoration; no per-block INFO logging was introduced.
- Persistence and failover: local cache serialization and asynchronous write/delete semantics are preserved. No transaction, EditLog, FE failover or authoritative-data persistence logic changes.
- Writes and crash behavior: restoration rebuilds cache state through the existing insertion/normalization path. Existing pending-delete/background-snapshot limitations are not introduced by this change; no distinct new atomicity or crash defect was established.
- FE/BE variables: none added; no propagation path is affected.
- Performance: restoration costs one seek plus O(blocks in this file) work under the existing cache mutex, as required for whole-file restoration. No concrete excessive-work failure or new deadlock was established; large-file latency was not measured.
- Other invariants: no new authoritative visibility, version or delete-bitmap behavior, ownership issue, header-hygiene mechanism, source-.cpp test inclusion or glibc dependency was identified. Normal, fully-covered and probe-based reads retain remote fallback on local cache errors.
This was static inspection only: no builds, tests, source modifications or runtime experiments were performed. All changed-file review scope is covered; runtime behavior beyond the inspected paths remains unverified.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 29892 ms |
### What problem does this PR solve?
Issue Number: None
Problem Summary: Peer cache requests carried a tablet ID in rowset_meta, but
the server left CacheContext.tablet_id at zero. During startup, lazy cache
restoration therefore queried the wrong metadata prefix and missed persisted
blocks. Pass the supplied tablet ID to get_or_set while preserving the zero
default for older clients that omit rowset_meta. Cover both request versions
with a restart test, and verify that background restoration preserves cache
accounting and the block objects already restored synchronously.
### Release note
Peer reads can restore persisted cache blocks under the requested tablet
before background cache loading finishes.
### Check List (For Author)
- Test: Unit Test
- Reproduced the peer startup miss before the fix; the legacy request passed.
- Passed 65 related ASAN unit tests using run-be-ut.sh, including peer
reads, cache metadata, all five lazy-restore cases, and both peer
startup request versions.
- Passed clang-format v16, check-format, build hygiene, and git diff --check.
- clang-tidy: no diagnostics on modified source lines; test checks passed.
The existing peer handler cognitive-complexity warning remains (58 > 50).
- Used GLIBC_COMPATIBILITY=OFF for the local unit-test link, as in the
preceding cache restoration validation; no production build options changed.
- Behavior changed: Yes; peer requests use their supplied tablet ID for cache restoration.
- Does this need documentation: No
Contributor
TPC-DS: Total hot run time: 152839 ms |
Contributor
ClickBench: Total hot run time: 24.78 s |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29385 ms |
Contributor
TPC-DS: Total hot run time: 152119 ms |
Contributor
ClickBench: Total hot run time: 24.46 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
gavinchou
approved these changes
Oct 16, 2026
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all 12 changed files at b562478 and their relevant callers, persistence paths, startup workers and tests. No verified newly introduced defect was found. There are no existing inline comments or independently confirmed existing P0/P1 findings. No additional user focus was supplied.
The review converged after round 1: both complete-scope reviewers and the separate startup-risk reviewer returned NO_NEW_VALUABLE_FINDINGS, all investigated concerns have evidence-backed dispositions, and the main final sweep found no uncovered changed file or unresolved candidate.
Critical checkpoint conclusions:
- Goal and evidence: The new tablet/hash prefix lookup removes dependence on an uninitialized offset and restores all persisted blocks for that identity in each of the three absent-file startup lookup APIs. Added tests inspect both nonzero offsets, preserved metadata, completed-startup bypass and actual peer-returned bytes. These tests were read, not executed.
- Scope and reuse: The change is focused. Shared prefix encoding is reused by serialization and range lookup; all renamed storage overrides and callers are consistent.
- Concurrency and locks: Foreground lookup holds the cache mutex throughout the missing-hash check and direct restoration. Background loading scans outside that mutex and merges under it using the existing per-offset deduplication. The absent-hash assertion is supported by the callers. No introduced lock-order inversion was found. Full-file RocksDB work remains inside the existing cache critical section.
- Lifecycle and memory: Iterators have local ownership; storage outlives synchronous scans and joins the background loader during destruction. Existing FileBlock/LRU ownership and accounting are reused. Test cleanup resumes paused loaders before destroying caches. No new cross-translation-unit initialization dependency or ownership cycle was found.
- Configuration: No configuration items or dynamic-update behavior were added.
- Compatibility: Persisted key bytes, version, value encoding and the existing tablet-wide range API are unchanged. The internal virtual-method rename updates both storage implementations. Optional peer metadata is read without changing the RPC schema; missing metadata retains tablet zero.
- Thrift/protobuf IDs: No definition changes; cross-branch field-ID comparison is not applicable.
- Parallel paths: get_or_set, fully-covered lookup, probe, filesystem and memory storage, peer reads, fill, and the tablet-wide TTL consumer were inspected. Existing startup trigger conditions remain unchanged. Peer identity limitations below are pre-existing.
- Conditional checks and errors: The assertion reflects a locked invariant. Null iterator handling corresponds to database initialization failure, and decode checks follow background-loader practice. No newly introduced silent-success failure path was established.
- Test coverage: New unit scenarios cover neighboring file/tablet prefixes, misses, aligned and unaligned non-probe reads, aligned probe, cache type/size/tablet preservation, background merge identity/accounting, completed-open skip, and peer requests with/without metadata. No cluster, corruption, TTL, upgrade or large-file performance test was executed.
- Expected results: No golden/result files changed. Added assertions agree with the inspected API behavior and fixture lifecycle.
- Observability: Existing lookup/probe/peer latency and cache counters cover the modified calls; iterator/decoding failures are logged. No additional critical observability requirement was established.
- Persistence and failover: This changes cache recovery reads. Serialization, async metadata writes and durable finalize/delete ordering are unchanged; no EditLog, transaction protocol or FE failover logic is involved.
- Writes and crash atomicity: Cache metadata publication remains serialized by the cache mutex. Existing stale-snapshot and asynchronous deletion windows were traced and were not established as new regressions.
- FE/BE variables: None added; the peer handler consumes the existing optional rowset metadata.
- Performance: A prefix seek streams only the selected file's records, with O(file blocks) cell restoration and no second full-file collection. Startup mutex latency and memory cost were not measured; no concrete new performance defect was verified.
- Other invariants and BE checks: No new transaction visibility, delete-bitmap, nullable-column, libc/linker, PCH, template-family or source-inclusion change. The read-only build-support/check-build-hygiene.sh check passed for header hygiene, extern-template pairing and unity skip coverage.
Limits: This was a static code review with no builds or functional tests. Historical peer-filled tablet-zero records can still miss a positive-tablet synchronous scan, and the unchanged sequential peer helper still defaults its outgoing tablet ID to zero. The patch does not migrate those keys or repair that sender; the old startup point lookup was already unreliable. Partial background loading and stale metadata windows also predate this patch. The added matching-identity tests do not prove recovery across those identity mismatches. These are documented existing limits, not unresolved introduced-defect candidates or new inline findings.
Review converged after 1 round(s).
3 of 7 tasks
bobhan1
pushed a commit
to bobhan1/doris
that referenced
this pull request
Nov 6, 2026
…pache#69116) Related PR: apache#57072, apache#65658 Problem Summary: During BE startup, get_or_set, the fully-covered lookup, and probe attempt to restore an absent file before background metadata loading completes. Their FileCacheKey leaves offset uninitialized, while the storage implementation now performs an exact RocksDB lookup for one block. A file with multiple persisted blocks can therefore miss existing cache data or restore only one block. Query metadata by tablet and file hash prefix, restore all blocks, and assert that the file hash is absent when entering synchronous restoration. Keep caller trigger conditions, key encoding, and existing tablet-range queries unchanged. Fix reuse of persistent file cache blocks while BE startup metadata loading is in progress. (cherry picked from commit e9b3373)
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
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.
Related PR: #57072, #65658
Problem Summary: During BE startup, get_or_set, the fully-covered lookup, and probe attempt to restore an absent file before background metadata loading completes. Their FileCacheKey leaves offset uninitialized, while the storage implementation now performs an exact RocksDB lookup for one block. A file with multiple persisted blocks can therefore miss existing cache data or restore only one block. Query metadata by tablet and file hash prefix, restore all blocks, and assert that the file hash is absent when entering synchronous restoration. Keep caller trigger conditions, key encoding, and existing tablet-range queries unchanged.
Fix reuse of persistent file cache blocks while BE startup metadata loading is in progress.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)