[feature](query-cache) Extend stale-entry incremental merge to cloud mode#65788
Open
asdf2014 wants to merge 2 commits into
Open
[feature](query-cache) Extend stale-entry incremental merge to cloud mode#65788asdf2014 wants to merge 2 commits into
asdf2014 wants to merge 2 commits into
Conversation
…mode The base incremental merge (apache#65482) only ran in the shared-nothing (local storage) deployment. This extends it to the compute-storage decoupled (cloud) mode. In cloud mode a tablet's rowset list, and for merge-on-write tables its delete bitmap, are a lazily synced copy of the meta service. The incremental decision runs in operator init, before the scan node's own sync round, so it must first bring each stale tablet's local view up to the queried version and only then capture the delta. Decision path: - _presync_cloud_delta_tablets fans the per-tablet meta-service syncs out in parallel (reusing init_scanner_sync_rowsets_parallelism) and waits on them with a fast-fail budget instead of blocking. The decision runs on the bounded query-admission pool (light_work_pool), so a meta-service brownout that stalled these RPCs for the full retry budget could exhaust the pool and reject query admission cluster-wide. If the fan-out does not finish within query_cache_decision_sync_timeout_ms (new cloud BE config, default 2000, changeable at runtime) the decision abandons the wait and falls the whole instance back to a full recompute. A value <= 0 disables cloud incremental merge as a fail-safe. - The sync brings the delete bitmap up to the queried version (sync_delete_bitmap=true), so the history-rewrite classification reads exactly what the merge-on-write read path sees. The later scan-node sync takes the same no-op shortcut (its local view is already at the queried version), so it issues no extra meta-service RPC and the RPC count per query does not grow. - While the classification reads the cached-side delete bitmap, the cached rowsets are pinned so a concurrent unused-rowset GC cannot retire the evidence. Both engines drop a retired rowset's bitmap only once nothing else references it, so raising the use count blocks the drop in either deployment mode. Correctness guard: a query that reads a version-inexact view (query_freshness_tolerance_ms or enable_prefer_cached_rowset) does not use incremental merge, since the delta capture always targets the exact queried version. The exclusion is a deliberately mode-agnostic gate. On cloud such a query also skips the query cache write-back, so no entry whose content mismatches its version stamp is ever created. Observability: new counter query_cache_decision_sync_time_ms records the wall time the decision spent syncing tablet views before capturing the delta. It only advances in cloud mode. Tests: BE unit tests cover the cloud decision paths (sync-then-capture, the timeout fallback, the merge-on-write bitmap rewrite check, the non-cloud-tablet and load-failure fallbacks, the no-op shortcut, and the write-back suppression). The FE QueryCacheNormalizerTest covers the freshness and prefer-cached gate. The query_cache_incremental regression suite now runs in cloud mode too, with auto-compaction disabled on its tables so the stale-hit metric assertions stay deterministic.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
11 tasks
Member
Author
|
run buildall |
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.
Requesting changes. The PR's goal—reuse stale query-cache entries through cloud incremental merge—is valuable, but the current implementation can classify MOW history against an incomplete delete bitmap, leaves timed-out metadata work without query/engine ownership, and contains mode/upgrade/test regressions.
Critical checkpoint conclusions:
- Goal and proof: the happy-path tests exercise cloud incremental decisions, but they do not prove the rowset-current/bitmap-incomplete state or cancellation/shutdown invariants, so the goal is not safely accomplished.
- Scope and focus: the 14-file change is generally focused on cache decisions, cloud sync, metrics, and tests; the blockers are within that intended scope.
- Concurrency and locks: per-tablet
_sync_meta_lockordering remains consistent and candidate construction stays outside the fragment-wide lock, but each query can launch an unbounded detached fanout. New driver/worker bthreads have no query/background thread context or cancellation owner. - Lifecycle/static initialization: there is no new cross-TU static-initialization dependency. The non-intuitive future/bthread lifecycle is unsafe because timeout destroys only the future while callbacks retain
CloudTabletobjects referencing an engine that shutdown does not join them before destroying. - Configuration: the new mutable timeout is read dynamically, but a non-positive value still launches work before immediately abandoning it; it must not create ownerless work.
- Compatibility: no storage format or symbol serialization changes are introduced, but rolling old-FE/new-BE requests can bypass freshness/prefer behavior because the upgraded BE removed the cloud fallback without its own compatibility gate.
- Parallel paths: local scans ignore both cloud knobs, and cloud MOW ignores prefer-cached-rowset, yet raw values disable incremental/write-back on those exact paths. Full-compaction tablet materialization is also a parallel sync path that can omit the bitmap.
- Conditional invariants: endpoint/version checks are well explained, but
_max_version >= query_versionis not proof of delete-bitmap completeness; the comments acknowledge this limitation without making the MOW decision conservative. - Error handling: sync statuses are checked and failure reasons/logs carry useful tablet context. The timeout path's defect is ownership/cancellation, not a silently ignored
Status. - Memory safety/accounting: shared result storage avoids a frame-use-after-free, but callbacks can outlive
CloudStorageEngine, and their metadata/delete-bitmap allocations lack the required bthread memory-tracker attachment. - Data correctness: visible delta endpoints are checked, but missing old bitmap markers can make a rewrite appear append-only and publish an incorrect version-stamped cache entry.
- Tests and results: BE/FE unit coverage and a regression were added, but the two critical negative lifecycle/bitmap cases are absent and the cloud metric oracle is routing-dependent. CheckStyle and the reported Cloud UT status pass; both Clang Formatter checks fail and a local clang-format-16 dry run reproduces changed-file violations. macOS BE UT fails before compilation because its runner exposes JDK 25 instead of required JDK 17; TeamCity compile/BE UT/FE UT/performance checks are still pending. Per the review-runner contract, no builds or test suites were launched locally.
- Observability: the new stale-hit/fallback/sync-time metrics and sampled logs are useful, but aggregating BE-local counters cannot prove that an unpinned query used a stale entry.
- Transaction, persistence, and writes: no EditLog, transaction, or durable metadata protocol changes are present. Query-cache publication is the relevant write and is unsafe in the incomplete-bitmap sequence; BE crash/shutdown can also intersect detached sync work.
- FE/BE variables: no new thrift field is added; existing query options are reused. Same-version FE/BE use is aligned, but mixed-version behavior needs a BE-side guard.
- Performance: per-instance parallelism reduces healthy latency, but no global bound exists across timed-out queries; raw-option gating can also eliminate cache fills/reuse on exact paths.
- BE null/nullable handling: not applicable; no column-nullability conversion or row-level nullable access is changed.
- Other issues: two formatter checks are currently red. No additional user focus was supplied beyond the complete PR review.
Contributor
TPC-H: Total hot run time: 29965 ms |
Contributor
TPC-DS: Total hot run time: 178153 ms |
Contributor
ClickBench: Total hot run time: 25.17 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Follow-up on PR apache#65788 addressing the review of the cloud incremental merge change. No storage format or symbol change; the only wire change is one optional, appended thrift field. Ownership and bound of the cloud decision pre-sync: - The per-tablet decision syncs no longer run on raw detached bthreads. They run on a dedicated, bounded, engine-owned ThreadPool (query_cache_delta_sync_pool). stop() drains it first, before the compaction and delete-bitmap pools its sync_rowsets callbacks depend on and before ~CloudStorageEngine destroys _meta_mgr/_tablet_mgr, so a task that outlived a timed-out query always runs against a live engine and none can survive it. - The pool has a fixed width and a bounded queue (config query_cache_delta_sync_ thread / query_cache_delta_sync_max_pending_tasks, both validated at startup), so a meta-service brownout can neither spawn an unbounded fanout nor grow the backlog without bound. Its workers are pre-started at engine construction and verified present with a CHECK on the worker count (ThreadPool::init swallows per-thread start failures, so a successful build alone proves nothing): submit is therefore pure enqueue and never creates a thread on the query admission path. - One absolute deadline, anchored at the fan-out start, covers submission plus the wait, so nothing can re-grant the budget. A shared abandoned flag, set when the wait expires, lets a not-yet-started task skip its RPC so the queue drains fast. A non-positive timeout launches nothing at all. - Each slot is claimed via an atomic exchange BEFORE any sync RPC, so exactly one of the task and the inline submit-failure path owns the RPC, the result slot, and the latch count-down; an enqueue-then-fail task a later worker picks up loses the claim and cannot fire a sync for an already-settled query. - The capture loop consumes each tablet's recorded fallback reason BEFORE its own get_tablet, and a worker-side tablet-load failure publishes a reason instead of leaving its slot empty, so no fallback route can push the synchronous cache-miss meta-service load onto the admission thread outside the deadline. Cloud MOW write-back and mixed-version: - BE no longer keys write-back on the raw prefer-cached knob for cloud MOW. FE resolves the selected index MOW state before the knob gate and reports it via a new optional field TQueryCacheParam.is_merge_on_write, and BE keeps write-back for the version-exact MOW read. The optional field is the mixed-version guard: old FE to new BE reads its default false and takes the conservative branch, new FE to old BE ignores it. Tests and regression determinism: - The regression stale-hit and fallback metric checks are gated to the shared-nothing path, sampling included; an aggregated BE-local counter cannot prove a specific cloud query used a stale entry when routing is topology-dependent. Cloud correctness is proven by result consistency plus deterministic BE unit tests. - New BE tests: pool-rejects-submit fallback, rejection-plus-cache-miss proving zero admission-thread meta loads, worker-load-failure proving a single worker-side load, non-cloud-engine fallback, engine-stopped fallback, decision-sync timeout fallback, cloud MOW prefer write-back, and cloud sync-completeness history-rewrite detection.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes.
One new inline comment flags the cross-fragment presync stampede: fragment-local decision maps do not coalesce identical stale-key work at the BE-global cache/pool boundary. The remaining blocking defects are already precisely anchored in existing discussions, so I did not duplicate them:
- P1: require an independent delete-bitmap completeness watermark. A cloud MOW tablet can be cold-loaded through version N with
sync_delete_bitmap=false; the later bitmap-enabled presync then returns on_max_version >= Nalone. A rewrite marker in(C,N]can remain absent, so the incremental classifier treats a history rewrite as append-only, merges the cached old row with its replacement, and writes a poisoned version-N cache entry. The new tests either force the sync or hand-install the bitmap and do not cover rowsets-current/bitmap-missing. - P1: make the freshness/prefer behavior safe across rolling FE/BE upgrades. Optional
is_merge_on_write=falseis structurally wire-compatible but does not protect either behavior boundary: an old FE can still request the new BE's exact-delta path while a warmed-read knob is active, and a new FE can send a MISS to an old BE whose unconditional write-back path stamps a potentially inexact scan result with the requested version. - The task ownership and shutdown portion of the presync lifecycle thread is fixed by the bounded engine-owned pool and shared state. Its admission-latency portion remains: fragment preparation waits synchronously for up to two seconds on a
brpc_lightworker during a meta-service brownout.
Critical checkpoint conclusions:
- Goal and proof: the change successfully enables cloud DUP/same-version MOW incremental decisions and suppresses unsafe same-version cache fills under freshness/prefer options, with broad unit and regression coverage. It does not yet prove cloud MOW delete-bitmap completeness, rolling-version safety, or globally coalesced bounded admission, so the overall goal is incomplete.
- Scope and clarity: all 17 changed files are related to incremental query-cache classification, its bounded cloud presync, protocol signal, metrics, and tests. The implementation is generally well explained and focused.
- Concurrency and thread safety: each fragment-local decision map is locked only for lookup/publication while metadata/RPC work stays outside the lock. The fan-out uses per-slot atomics, shared latch/result ownership, and a bounded pool; no lock-order or deadlock defect was found. CacheSource and OlapScan local states are prepared sequentially within one fragment instance, but separate concurrent fragment contexts have private maps while sharing
QueryCache::instance()and the same presync pool; that real production boundary permits the newly reported same-key stampede. Synchronous blocking of the light admission pool also remains, as noted above. - Lifecycle: the new pool is engine-owned, rejects/clears queued work on shutdown, joins running callbacks before tablet/meta dependencies are destroyed, and timed-out callbacks retain shared slot/latch state. Worker threads receive the normal default thread context. No circular ownership or cross-translation-unit static initialization issue was found.
- Configuration:
query_cache_decision_sync_timeout_msis dynamic and each invocation observes it; non-positive values disable cloud incremental safely. Pool width and queue capacity are startup-only, which matches pool construction, and invalid values fail loudly. The timeout still needs an execution model that does not block light admission workers. - Compatibility: Thrift field addition 9 is optional and preserves decoding, but behavior is not safe in either mixed-version direction described above. This is blocking.
- Parallel paths and conditions: local storage remains exact and does not apply cloud-only knobs; cloud DUP and MOW paths were both traced; selected-index MOW derivation and the same-version write-back conditions match storage capture semantics. The missing bitmap watermark is the parallel metadata-state path not covered by the new condition.
- Tests and expected results: FE tests cover selected-index MOW and both knobs; BE tests cover write-back gates, cloud success/fallback, pool rejection, timeout, fan-out, and MOW rewrite detection; the regression test adds correctness/metric phases and correctly gates topology-dependent cloud metrics. Missing negative cases are rowsets-current/bitmap-missing, both rolling-version matrices, and two distinct runtimes contending for one stale global key while an unrelated key seeks admission. No generated result file needed changing. Per the review contract I did not run local builds or tests; formatter/checkstyle and Cloud UT pass, macOS BE UT failed in environment setup due the Java-version mismatch, and BE UT/FE UT/compile were still pending at review time.
- Observability: stale-hit, fallback, and presync-time metrics plus fallback reasons provide useful coverage without adding hot-path INFO logging. No further metric is required for the reviewed behavior.
- Persistence and transactions: no EditLog, durable metadata format, transaction, or failover state is changed. Query-cache write-back is ephemeral, but the blocking correctness paths can still poison or misuse cached results.
- Data writes and crash behavior: there are no storage-engine data writes. Per-runtime decision publication and pinned-handle ownership are safe; pool shutdown does not leave callbacks using destroyed engine state.
- FE-to-BE variables: the sole current FE construction path sets
allow_incrementalandis_merge_on_write, and same-version propagation is complete. Mixed-version semantics remain unresolved because the new behavior cannot be enforced by the old endpoint. - Performance: the fixed-width, bounded queue caps meta-service concurrency and backlog; abandoned tasks skip new RPCs. A large instance performs one job per tablet, and concurrent identical fragments can submit that entire fan-out repeatedly because coalescing is fragment-local; the tablet lock acts only after pool admission. This avoidable queue amplification and the synchronous light-pool wait are availability risks.
- Other checks: status failures are converted to conservative full recomputation with tablet-level reasons; shared ownership is safe; no nullable-column or unrelated storage-format concern applies.
Focus response: no additional user-specified focus area was provided; I completed the full changed-file review.
| CloudStorageEngine* engine_ptr = engine; | ||
| for (size_t i = 0; i < scan_ranges.size(); ++i) { | ||
| int64_t tablet_id = scan_ranges[i].scan_range.palo_scan_range.tablet_id; | ||
| Status submit_st = pool.submit_func([tablet_id, current_version, abandoned, i, engine_ptr, |
Contributor
There was a problem hiding this comment.
[P1] Coalesce presync across fragment runtimes
QueryCacheRuntime only coalesces inside one PFC, but each concurrent query fragment has a private decision map while all of them read the BE-global QueryCache::instance(). Under default single-primary placement (or a one-BE compute group), identical query-cache queries reach the same BE and stale key, so each runtime independently executes this per-tablet submit loop. While metadata syncs are slow, three default-limit 768-tablet instances submit 2304 jobs into the default 16-worker/2048-pending pool: at least 240 submissions reject even though any one fan-out fits, forcing avoidable full recomputation and denying unrelated keys admission. The tablet sync lock acts only after pool admission. Please single-flight sync completion globally by (cache_key, current_version) while retaining per-query deadlines, fallback, and delta-source capture, and test distinct runtimes plus an unrelated key.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29527 ms |
Contributor
TPC-DS: Total hot run time: 178785 ms |
Contributor
ClickBench: Total hot run time: 25.47 s |
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
2 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: close #xxx
Related PR: #65482
Problem Summary:
Extends the stale query cache entry incremental merge (#65482) from the shared-nothing (local storage) deployment to the compute-storage decoupled (cloud) mode.
In cloud mode a tablet's rowsets, and for merge-on-write tables its delete bitmap, are a lazily synced copy of the meta service. The base change refused incremental merge on cloud outright. This brings the same optimization to cloud by syncing each stale tablet's view to the queried version before capturing the delta:
query_cache_decision_sync_timeout_ms, new cloud BE config, default 2000 ms, changeable at runtime). The decision runs on the bounded query-admission pool, so on a meta-service brownout it falls back to a full recompute rather than holding that thread. A value <= 0 disables cloud incremental merge as a fail-safe.query_freshness_tolerance_msorenable_prefer_cached_rowset) is excluded from incremental merge and, on cloud, from the cache write-back, so no entry whose content mismatches its version stamp is ever created.Observability: new counter
query_cache_decision_sync_time_ms(only advances in cloud mode) records the wall time the decision spent syncing tablet views before capturing the delta.Release note
Query Cache incremental merge now works in the compute-storage decoupled (cloud) mode. New BE config
query_cache_decision_sync_timeout_ms(default 2000, changeable at runtime) bounds the decision's tablet view sync; on timeout the query falls back to one full recompute.Check List (For Author)
Test
query_cache_incrementalregression suite and a four-phase benchmark (no cache, exact hit, incremental merge, incremental off) on a real compute-storage decoupled cluster (FoundationDB + meta service + MinIO): the stale-hit and fallback metrics match the shared-nothing baseline, and the decision sync time stays in the tens of milliseconds.Behavior changed:
cloud rowset sync failed,cloud rowset sync timed out).Does this need documentation?
Check List (For Reviewer who merge this PR)