Uh oh!
There was an error while loading. Please reload this page.
branch-4.1: [fix](filecache) skip redundant ttl scans for non-TTL tablets (pick #65434) - #65429
Conversation
hello-stephen
commented
Jul 9, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
freemandealer
commented
Jul 9, 2026
run buildall |
freemandealer
commented
Jul 9, 2026
/review |
There was a problem hiding this comment.
Requesting changes for one file-cache TTL correctness issue.
Critical checkpoints:
- Changed files reviewed:
be/src/io/cache/block_file_cache_ttl_mgr.cppandbe/test/io/cache/block_file_cache_ttl_mgr_test.cpp, plus relevant cache load and cache-type transition paths. - Existing review context: no existing inline review threads or replies were present, so this is not a duplicate.
- User focus: no additional user-provided review focus.
- Repo instructions: no AGENTS.md files were listed; a repo-local code-review SKILL.md was not present in the expected paths, so I followed the review prompt and ledger workflow.
- Subagent conclusions:
optimizer-rewriteproduced no candidates and was dismissed with evidence;tests-session-configproducedmain-001, accepted as the inline comment below; convergence round 1 ended with both subagents replyingNO_NEW_VALUABLE_FINDINGSfor the same ledger/comment set. - Validation: static review only. The checkout is grafted and missing the base object for local diff checks, and
thirdparty/installedplusthirdparty/installed/bin/protocare absent, so I did not run BE unit tests.
Uh oh!
There was an error while loading. Please reload this page.
35323a5 to
7ae7df1Comparefreemandealer
commented
Jul 9, 2026
/review |
freemandealer
commented
Jul 9, 2026
run buildall |
There was a problem hiding this comment.
Automated review result: request changes.
Critical checkpoints:
- Scope: live PR head 7ae7df1; GitHub reports two changed files: be/src/io/cache/block_file_cache_ttl_mgr.cpp and be/test/io/cache/block_file_cache_ttl_mgr_test.cpp.
- Existing review context: I considered the existing restart/persisted-TTL thread. The inline comment below is a distinct current-patch follow-up because the new periodic reconcile addresses the old _ttl_info_map gate but still depends on missing tablet-id registration.
- User focus: no additional user-provided review focus was supplied.
- Validation: git show --format= --check HEAD -- be/src/io/cache/block_file_cache_ttl_mgr.cpp be/test/io/cache/block_file_cache_ttl_mgr_test.cpp passed. I did not run the BE unit test locally because this grafted checkout is missing thirdparty/installed and thirdparty/installed/bin/protoc; the PR body reports the author's BlockFileCacheTtlMgrTest run.
Subagent conclusions:
- optimizer-rewrite proposed optimizer-rewrite-001, accepted as inline-001.
- tests-session-config proposed tests-session-config-001, merged as duplicate/supporting evidence for inline-001 rather than a second comment.
- Convergence round 1 ended with both live subagents reporting NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set.
Uh oh!
There was an error while loading. Please reload this page.
freemandealer
commented
Jul 9, 2026
run beut |
hello-stephen
commented
Jul 9, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jul 9, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
not introduced by this PR
Uh oh!
There was an error while loading. Please reload this page.
### What problem does this PR solve? Issue Number: None Related PR: #65429 Problem Summary: Pick #65429 to master. The file cache TTL manager keeps registered tablet ids in `_tablet_id_set`. For ordinary non-TTL tablets, the old `ttl_seconds <= 0` path always set `need_convert_from_ttl = true`, so every update round scanned cached blocks even when the tablet had never been recorded in `_ttl_info_map`. This PR reduces the repeated CPU work for registered non-TTL tablets: - If a tablet was previously tracked as TTL, it is still converted back to NORMAL immediately when TTL is disabled. - If a tablet has no prior TTL info, the manager skips the per-round cached block scan. - A low-frequency reconciliation scan is kept every 20 update rounds, so cleanup behavior is preserved while avoiding scans on every round. With the default `file_cache_background_ttl_info_update_interval_ms = 180000`, ordinary non-TTL tablets avoid the repeated per-round block scan that previously ran every 3 minutes.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
The file cache TTL manager keeps registered tablet ids in
_tablet_id_set. For ordinary non-TTL tablets, the oldttl_seconds <= 0path always setneed_convert_from_ttl = true, so every update round scanned cached blocks even when the tablet had never been recorded in_ttl_info_map.This PR reduces the repeated CPU work for registered non-TTL tablets:
With the default
file_cache_background_ttl_info_update_interval_ms = 180000, ordinary non-TTL tablets avoid the repeated per-round block scan that previously ran every 3 minutes.Release note
None
Check List (For Author)
Unit test:
DORIS_TOOLCHAIN=clang DISABLE_BE_JAVA_EXTENSIONS=ON ENABLE_INJECTION_POINT=ON ENABLE_CACHE_LOCK_DEBUG=0 ENABLE_PCH=0 sh run-be-ut.sh --run --filter='BlockFileCacheTtlMgrTest.*'Result: 5 tests passed.
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)