Uh oh!
There was an error while loading. Please reload this page.
branch-4.1: [opt](cloud) cache cluster id per query and drop redundant locks on getBackendId hot path #63636 - #64276
Conversation
hello-stephen
commented
Jun 9, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
Pull request overview
This PR backports #63636 to optimize cloud-mode hot paths by resolving the current compute group (cluster) id once per request and reusing it across tablet/replica placement lookups, while also refactoring cluster-id resolution logic into CloudSystemInfoService.
Changes:
- Add
CloudSystemInfoService.getCurrentClusterId()/resolveClusterIdByName()and a lightweightcontainsCloudCluster()helper, with a new unit test. - Hoist and cache cluster-id resolution in high-tablet-count code paths (planner scan/sink and FE partition operations) to avoid repeated per-replica resolution.
- Refactor cloud tablet/replica backend-id selection APIs to support passing a pre-resolved cluster id, and improve exception chaining in
OlapTableSink.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| fe/fe-core/src/test/java/org/apache/doris/cloud/system/CloudSystemInfoServiceTest.java | Adds coverage for the new containsCloudCluster() helper. |
| fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java | Caches cluster id per request when resolving CloudTablet BE-path maps (create/replace partition). |
| fe/fe-core/src/main/java/org/apache/doris/planner/OlapTableSink.java | Uses cached cluster id for CloudTablet BE-path map resolution; preserves ComputeGroupException as cause. |
| fe/fe-core/src/main/java/org/apache/doris/planner/OlapScanNode.java | Caches cluster id when resolving CloudReplica backend ids for scan range location selection. |
| fe/fe-core/src/main/java/org/apache/doris/cloud/system/CloudSystemInfoService.java | Centralizes current-cluster-id resolution and adds containsCloudCluster(). |
| fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudTablet.java | Adds getNormalReplicaBackendPathMapByClusterId() and routes default lookup through getCurrentClusterId(). |
| fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudReplica.java | Adds getBackendIdWithClusterId() and proc-display mapping logic for colocated replicas. |
| fe/fe-core/src/main/java/org/apache/doris/alter/AlterJobV2.java | Updates comment to reflect new cluster-id resolution entry point. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
1c977ab to
093a26fCompareliaoxin01
commented
Jun 9, 2026
run buildall |
e79a8fa to
38bc898Compareliaoxin01
commented
Jun 9, 2026
run buildall |
…etBackendId hot path (apache#63636) `CloudReplica.getBackendId` was the dominant FE hotspot in cloud-mode query/load planning. On a profiled query it accounted for **65.6% of total FE CPU samples** (async-profiler). Every replica in the plan re-ran the full cluster-id resolution pipeline even though the resolved cluster id is identical for every tablet in the same request. Call breakdown from the flame graph: ``` Tablet.getNormalReplicaBackendPathMap 68.6% └─ CloudReplica.getBackendId 65.6% └─ getCurrentClusterId 61.5% ├─ getCloudClusterIdByName 35.6% │ ├─ getCloudClusterNames 24.7% (rw-lock + ArrayList + stream.sorted) │ └─ waitForAutoStart 9.7% (StopWatch.start/stop even on NORMAL) ├─ getPhysicalCluster 14.5% │ └─ getComputeGroupByName 14.5% (rw-lock around ConcurrentHashMap) └─ getCloudStatusByName 8.3% └─ getCloudStatusByIdNoLock 14.0% (two stream passes, String.valueOf per BE) ``` `ReadLock.unlock` consistently outweighed `ReadLock.lock` in the profile -- a classic cache-line bouncing signature, meaning the read-lock CAS was already operating in the non-linear regime where adding concurrent threads disproportionately hurts throughput. With high tablet counts the call rate (`tablets × replicas × concurrent_queries`) easily reached 100k+ lock/unlock per second per FE, putting the FE one nudge away from a metastable collapse. - **`CloudReplica.getCurrentClusterId`** becomes `public static` so callers can resolve once per request. Adds `getBackendIdWithClusterId(String)` that bypasses the per-replica pipeline. - **`OlapTableSink.createLocation`** and **`FrontendServiceImpl.{createPartition, replacePartition}`** resolve the cluster id once before iterating tablets and pass it down via the new `CloudTablet.getNormalReplicaBackendPathMap(String)` overload. For a 10k-tablet query this collapses 10k full pipelines into 1. - **`CloudSystemInfoService.getComputeGroupByName`**: drop the rw-lock around `ConcurrentHashMap` reads, merge `containsKey+get` into a single `get`, guard the debug log behind `isDebugEnabled` (the map `toString` is expensive). - **`CloudSystemInfoService.containsCloudCluster`** (new): cheap existence check that replaces `getCloudClusterNames().contains(name)`. Avoids an ArrayList copy + stream filter + natural sort + collect under a read lock for a single existence query. - **`CloudSystemInfoService.getCloudStatusByIdNoLock`**: single-pass loop with a precomputed `NORMAL` constant in place of two stream pipelines that re-evaluated `String.valueOf(NORMAL)` per backend. - **`CloudSystemInfoService.waitForAutoStart`**: fast-path return when the cluster is already `NORMAL`, skipping the `withTemporaryNereidsTimeout` wrap and the `StopWatch.start/stop` inside `waitForClusterToResume` (the while loop never executed in that state but the wrapping still ran per call -- ~3% of total FE CPU in the profile). Semantics preserved on all paths: - `waitForAutoStart` NORMAL fast-path: the original code already had `existAliveBe = true` as initializer, so `waitForClusterToResume` was a no-op for NORMAL clusters. - `getComputeGroupByName` without rw-lock: the brief window between a rename's two map updates can now return `null`; same as the existing read-only paths everywhere else in this class that already access these maps without locks. - `containsCloudCluster` matches `getCloudClusterNames().contains(name)` for non-empty `name` (empty-name filtering in `getCloudClusterNames` was for the returned list, not for `.contains` semantics). The cluster id resolution is hoisted only on the no-BE-endpoint paths (the hot ones in the profile). The endpoint-resolved path in `FrontendServiceImpl` already has its own resolution logic and is untouched.
38bc898 to
169d403Compareliaoxin01
commented
Jun 9, 2026
run buildall |
hello-stephen
commented
Jun 9, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Jun 9, 2026
FE Regression Coverage ReportIncrement line coverage |
Uh oh!
There was an error while loading. Please reload this page.
Pick #63636