asdf2014 commented on PR #65788: URL: https://github.com/apache/doris/pull/65788#issuecomment-5023106076
Commit da0b255 resolves the three PR-scoped findings, and a full re-review of the pre-sync decision path added a few more guards. **[P1] Contain exceptions inside the pre-sync worker** The per-tablet task body is now wrapped in a try/catch (`doris::Exception` before `std::exception`, then a catch-all). A `std::system_error` from the single-flight loader, a `bad_alloc` inside `sync_rowsets`, or any other escape converts to a per-tablet fallback reason instead of reaching `std::terminate`. Covered by `presync_decision_sync_raise_falls_back` and `presync_decision_sync_raises_doris_exception_falls_back` (red-verified: reverting the catch reproduces the abort). **[P2] Keep the shared-state deleter non-throwing** The flight's O(tablets) buffers are no longer charged through a `shared_ptr` deleter that switched the thread tracker. The whole flight is charged once via `consume()`/`release()` on the query-cache `MemTrackerLimiter` (plain atomic counter ops, no allocation, no throw), released in `~PresyncFlight`. Every buffer is now a plain `make_shared`, so nothing runs on the non-throwing destruction path. **[P1] Keep the cloud sync wait off fragment admission** A soft cap bounds the concurrent decision-sync waiters admitted onto the light pool; over the cap the query skips incremental and recomputes in full. The cap is now derived from the actual light-pool width (the smaller of `query_cache_max_concurrent_decision_sync` and half of `brpc_light_work_pool_threads`, or `max(128, 4 * cores)` when unset), because a fixed cap would not bind on a configured-small pool, and a one-thread pool admits no waiter at all. The decision capture-site `get_tablet` is also cache-only now, so an evicted pre-synced tablet falls back rather than reloading synchronously on the admission thread. Covered by `presync_over_concurrency_cap_falls_back`, `presync_cap_clamps_to_configured_light_pool_width`, `presync_single_thread_light_pool_never_parks_sole_worker`, and `capture_site_get_tablet_is_cache_only_on_eviction` (all red-verified). **Delete-bitmap completeness (r3610138906)** Confirmed pre-existing and not introduced here. A full-compaction submission loads a MOW tablet with `sync_delete_bitmap=false` (`task_worker_pool.cpp`, `cloud_compaction_action.cpp`), and the `sync_rowsets` `_max_version` fast path does not re-check bitmap coverage. A normal cloud MOW scan issues the byte-identical sync (`olap_scan_operator.cpp`, default `sync_delete_bitmap=true`) and reads the same `tablet_meta()->delete_bitmap()` with no separate re-fetch, so it is equally exposed; both building blocks predate this PR (git blame: 2025-03 for the fast path, 2026-04 for the bitmap-less compaction load). The classification comment now states this explicitly instead of the earlier incorrect "no caller passes sync_delete_bitmap=false". The clean repair belongs in the storage layer (require bitmap coverage before the fast path returns, or keep bitmap-less loads out of the query-visible tablet cache) and would fix the normal scan and this path together; tracking it as a separate follo w-up rather than widening this change. BE UT and FE UT pass locally. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
