[fix](filecache) skip redundant ttl scans for non-TTL tablets#65434
[fix](filecache) skip redundant ttl scans for non-TTL tablets#65434freemandealer wants to merge 1 commit into
Conversation
freemandealer
requested review from
Gabriel39,
gavinchou,
liaoxin01 and
morningman
as code owners
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review completed for PR 65434 at e63d5d4.
I found one blocking issue in the TTL reconcile logic: the new full reconcile only handles tablet IDs already in the TTL manager's in-memory set, so persisted TTL blocks restored from metadata after restart can remain in the TTL queue when the tablet's current TTL is 0. See the inline comment.
Critical checkpoint conclusions: the change is small and focused, no new config is added, and lock ordering/thread structure is not materially changed. The lifecycle/compatibility checkpoint is not satisfied because persisted metadata restore/direct-load paths are parallel paths to request-time cell creation but do not register tablet IDs. Test coverage is incomplete because the new restart-style test manually registers the tablet ID instead of exercising production restore registration. No additional user-provided focus points were supplied.
Validation notes: I did not run local BE tests in this shallow/grafted checkout because thirdparty/installed is absent. The PR body reports BlockFileCacheTtlMgrTest.* passed on the source branch. GitHub's macOS BE UT failure exits before build due ERROR: The JAVA version is 25, it must be JDK-17, so I treated it as environment setup rather than evidence for this patch.
Subagent conclusions: optimizer-rewrite found no valuable optimizer/rewrite findings. tests-session-config proposed TSC-1, which I independently verified and accepted as the inline comment. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the final ledger/comment set.
| need_convert_from_ttl = true; | ||
| // Periodically reconcile blocks restored from persisted TTL metadata, | ||
| // because _ttl_info_map is rebuilt only in memory after restart. | ||
| need_convert_from_ttl = |
There was a problem hiding this comment.
This reconcile still misses the restart case described in the comment. The update loop only iterates tablet_ids_to_process, which is copied from _tablet_id_set; however startup restore from RocksDB goes through FSFileCacheStorage::load_cache_info_into_memory_from_db() -> mgr->add_cell(...), and BlockFileCache::add_cell() does not call register_tablet_id (the registration in this area only happens when split_range_into_cells() creates new request cells). After a BE restart, a persisted TTL block for a tablet whose current TTL is now 0 can therefore be loaded into the TTL queue without its tablet ID ever entering _tablet_id_set, so the 20-round reconcile will never scan or demote it unless a later cache miss happens to register the same tablet. The new test manually calls register_tablet_id, so it does not cover that production restore path. Please register restored/direct-loaded tablet IDs with the TTL manager or initialize the manager from the meta store, and add a restart-style test that does not manually register the tablet.
There was a problem hiding this comment.
not introduced by this PR
TPC-H: Total hot run time: 29260 ms |
freemandealer
dismissed
github-actions[bot]’s stale review
not introduced by this PR
TPC-DS: Total hot run time: 180190 ms |
ClickBench: Total hot run time: 25.11 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
2 participants
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 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 on source PR branch:
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.
Pick validation on master:
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)