[improvement](filecache) Adapt file cache queue consumption#63504
Open
freemandealer wants to merge 6 commits into
Open
[improvement](filecache) Adapt file cache queue consumption#63504freemandealer wants to merge 6 commits into
freemandealer wants to merge 6 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31083 ms |
Contributor
TPC-DS: Total hot run time: 170549 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
gavinchou
reviewed
May 26, 2026
gavinchou
reviewed
May 26, 2026
gavinchou
reviewed
May 26, 2026
gavinchou
previously requested changes
May 26, 2026
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: File cache hit ratio metrics are derived from global file cache read bytes, but warmup reads from manual warmup, periodic warmup, event-driven warmup, and rebalance-triggered warmup used to update the same counters as query reads. This polluted the query hit ratio. Mixed hit/miss reads could also be attributed to one source for the whole request. This change skips warmup updates to global file cache read metrics while preserving per-IOContext profile stats, records local/remote/peer bytes by actual returned bytes, and avoids updating metrics for failed reads. It also fixes direct-read partial continuation and no-warmup miss-only hit ratio refresh. After rebase, the warmup metrics UT exposed a separate ASAN issue because the test snapshot helper triggered all metric hooks, including a stale StorageEngine hook that captured a destroyed engine. The test now snapshots FileCacheMetrics directly, and StorageEngine deregisters its hook on destruction.
### Release note
File cache hit ratio metrics now exclude warmup reads.
### Check List (For Author)
- Test: Regression test / Unit Test
- 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=BlockFileCacheTest.cached_remote_file_reader_warmup_does_not_update_global_metrics
- 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='BlockFileCacheTest.cached_remote_file_reader*'
- Behavior changed: Yes. Warmup reads no longer contribute to global file cache read metrics used for query hit ratio; per-IOContext profile stats are preserved.
- Does this need documentation: No
Problem Summary: File cache background consumers used fixed intervals and batch sizes for LRU recorder log replay and _need_update_lru_blocks updates. When producers outpaced those consumers, backlog growth was hard to observe and could increase memory pressure. This change adds queue length metrics for LRU recorder log queues, exposes queue-size accessors, supports bounded LRU log replay, and makes both background consumers adapt their interval and batch size according to backlog watermarks. It also slices block LRU update work into smaller lock-hold batches and skips LRU log recording when tail-record retention is disabled.
None
- Test: Unit Test
- `CCACHE_DISABLE=1 DORIS_TOOLCHAIN=clang DISABLE_BE_JAVA_EXTENSIONS=ON ENABLE_INJECTION_POINT=ON ENABLE_CACHE_LOCK_DEBUG=0 ENABLE_PCH=0 EXTRA_CXX_FLAGS='-Wno-error=deprecated-literal-operator' sh run-be-ut.sh --run --filter=BlockFileCacheTest.test_lru_log_replay_bound_and_disable_record`
- `build-support/check-format.sh`
- `git diff --check`
- Tried `build-support/run-clang-tidy.sh --base origin/master --build-dir be/ut_build_ASAN`; it was blocked by pre-existing/file-level diagnostics and system header lookup errors before producing a clean result.
- Behavior changed: Yes. File cache background queue consumers can increase consume frequency and batch size when backlog crosses configured watermarks.
- Does this need documentation: No
Signed-off-by: zhengyu <zhangzhengyu@selectdb.com>
freemandealer
force-pushed
the
task-master-pick-file-cache-adaptive-queue-consu
branch
from
a3410c3 to
97a715b
Compare
freemandealer
requested review from
Gabriel39,
liaoxin01 and
morningman
as code owners
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29197 ms |
Contributor
TPC-DS: Total hot run time: 170457 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Member
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found one correctness issue in the bounded LRU log replay path.
Critical checkpoint conclusions:
- Goal/test: The PR improves file-cache queue consumption and metric accounting, and adds BE/regression coverage, but the bounded replay change can make persisted LRU dumps miss already-recorded events.
- Scope/minimality: The final version is mostly focused and removed the previously raised extra knobs/metrics.
- Concurrency/lifecycle: The issue is a lifecycle interaction between the replay thread and dump thread over the shadow queue and update counter; both use the recorder mutex for the shadow queue, but dump can still observe an intentionally partially replayed state.
- Config/compatibility: No remaining new configs in the final head; no storage format or FE/BE protocol compatibility concern found.
- Parallel paths: The concern applies to all four LRU queues because they all use the same bounded replay and dump path.
- Tests: Unit tests cover bounded replay itself, but do not cover dumping while replay backlog remains pending.
- Observability/performance: The new backlog metric/logging helps observe backlog, but does not prevent stale dumps.
- Transaction/persistence: This is persistence-related for file-cache LRU tail restore; a crash after a stale dump can restore an out-of-date LRU order.
User focus: No additional user-provided review focus was specified.
Signed-off-by: zhengyu <zhangzhengyu@selectdb.com>
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary for PR 63504 at 12268c6:
I did not find additional blocking issues beyond the existing review threads. The previously raised LRU dump/replay race is addressed in the current diff by draining pending recorder logs under _mutex_lru_log before collecting dump entries and by subtracting only the counted updates after a successful dump.
Critical checkpoint conclusions:
- Goal and proof: The PR narrows file-cache read metrics so warmup reads do not affect global read metrics, splits mixed local/remote/peer read bytes, converts queue length metrics to bvar::Status, and bounds/adapts LRU update/replay draining. Added BE unit tests and a cloud docker regression cover these paths.
- Small and focused: The actual GitHub PR diff is focused on BE file-cache metrics/LRU behavior and related tests.
- Concurrency: LRU recorder replay/dump/counter updates are protected by _mutex_lru_log. Block LRU updates drain outside the cache mutex and slice work under _mutex, keeping lock holds bounded. No new lock-order issue found in the reviewed paths.
- Lifecycle/static initialization: No new cross-TU static initialization dependency or special ownership cycle found. StorageEngine hook deregistration is paired in destructor.
- Configuration: No new config item remains in the actual PR diff; existing dynamic config reads are used directly.
- Compatibility: No storage format, Thrift, or FE/BE protocol compatibility issue found.
- Parallel paths: Direct read, indirect cached read, remote fallback, warmup, and mixed hit/miss paths are covered in code/tests.
- Special conditions: Warmup exclusion from global metrics is explicit; failure paths avoid publishing partial global stats.
- Test coverage: BE unit tests cover metric deltas, warmup exclusion, mixed reads, failed reads, and LRU dump/replay behavior. The cloud docker regression covers manual/periodic/event/rebalance warmup metric behavior. I did not run tests locally in this review.
- Observability: Queue backlog status metrics and warning logs are present; no additional observability gap found.
- Transaction/persistence/data correctness: The LRU dump persistence path now drains pending records before dumping, preserving restore consistency for counted records. No transaction path changes.
- Performance: Adaptive draining and lock slicing reduce backlog/lock pressure. Dump collection is done under the recorder mutex and disk IO is outside it.
User focus: No additional user-provided review focus was supplied.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
freemandealer
dismissed stale reviews from gavinchou and github-actions[bot]
done fix
Contributor
TPC-H: Total hot run time: 29742 ms |
Contributor
TPC-DS: Total hot run time: 169839 ms |
gavinchou
reviewed
Jun 3, 2026
| } | ||
|
|
||
| if (_no_warmup_num_hit_blocks->get_value() > 0) { | ||
| if (_no_warmup_num_read_blocks->get_value() > 0) { |
Member
Author
There was a problem hiding this comment.
a quick fix for div-by-zero bug
gavinchou
reviewed
Jun 3, 2026
gavinchou
requested changes
Jun 3, 2026
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29264 ms |
Contributor
TPC-DS: Total hot run time: 168476 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Member
Author
|
run p0 |
Member
Author
|
run cloud_p0 |
Member
Author
|
run nonConcurrent |
Member
Author
|
/review |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
There was a problem hiding this comment.
Review conclusion: no additional blocking issue found in the actual GitHub PR diff.
Critical checkpoint conclusions:
- Goal/test proof: The PR addresses adaptive file-cache queue consumption and queue visibility. BE unit tests cover bounded LRU log replay, dump-counter preservation, hard-cap drops, need-update LRU block behavior, and file-cache read metrics. A docker regression test is added for warmup read metrics.
- Scope/minimality: The GitHub PR file list is focused on BE file-cache code plus related tests. The earlier local-only unrelated FE/README diff was not part of GitHub's accepted PR diff.
- Concurrency: Reviewed the LRU recorder mutex usage, bounded replay, dump-drain-before-reset flow, block LRU update slicing under the cache lock, and the close condition-variable loops. No new distinct concurrency blocker found beyond the already-known/replied review threads.
- Lifecycle: Background threads are still joined in the cache destructor; the added storage-engine metric deregistration is appropriate for destructor lifecycle.
- Configuration: Extra adaptive knobs from earlier review context are no longer present; the remaining adaptive thresholds are internal constants. Existing mutable config values are still read in the background loops.
- Compatibility/storage/transactions: No storage format, FE/BE protocol, EditLog, or transaction compatibility issue identified.
- Parallel paths: File-cache read accounting now splits local/remote/peer bytes for direct, indirect, warmup, fallback, and mixed-hit paths; tests exercise these paths.
- Tests: Relevant BE unit coverage is present. I did not run tests in this review runner.
- Observability/performance: Queue length/drop metrics and high-backlog warnings are present; lock slicing reduces long cache-lock holds. No additional observability blocker found.
- User focus: No additional user-provided review focus was specified.
Existing review threads were checked first; I did not repeat the previously raised metrics, wait interval, or dump/replay race comments.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
|
skip buildall |
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.
Problem Summary: File cache background consumers used fixed intervals and batch sizes for LRU recorder log replay and _need_update_lru_blocks updates. When producers outpaced those consumers, backlog growth was hard to observe and could increase memory pressure. This change adds queue length metrics for LRU recorder log queues, exposes queue-size accessors, supports bounded LRU log replay, and makes both background consumers adapt their interval and batch size according to backlog watermarks. It also slices block LRU update work into smaller lock-hold batches and skips LRU log recording when tail-record retention is disabled.
None
CCACHE_DISABLE=1 DORIS_TOOLCHAIN=clang DISABLE_BE_JAVA_EXTENSIONS=ON ENABLE_INJECTION_POINT=ON ENABLE_CACHE_LOCK_DEBUG=0 ENABLE_PCH=0 EXTRA_CXX_FLAGS='-Wno-error=deprecated-literal-operator' sh run-be-ut.sh --run --filter=BlockFileCacheTest.test_lru_log_replay_bound_and_disable_recordbuild-support/check-format.shgit diff --checkbuild-support/run-clang-tidy.sh --base origin/master --build-dir be/ut_build_ASAN; it was blocked by pre-existing/file-level diagnostics and system header lookup errors before producing a clean result.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)