[fix](auto partition) restrict dummy load locations to current compute group - #66083
Open
sollhui wants to merge 7 commits into
Open
[fix](auto partition) restrict dummy load locations to current compute group#66083sollhui wants to merge 7 commits into
sollhui wants to merge 7 commits into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Automatic partition loads with no physical partitions create dummy tablet locations. The previous logic sampled from all alive backends in a cloud instance, allowing a backend from another compute group to be selected. Restrict candidates to load-available, non-decommissioning backends in the current compute group before choosing a dummy location.
### Release note
Fix automatic partition loads to choose dummy tablet locations from the current compute group.
### Check List (For Author)
- Test: Unit Test
- ./run-fe-ut.sh --run org.apache.doris.planner.OlapTableSinkTest
- DISABLE_BUILD_UI=ON ./build.sh --fe
- Behavior changed: Yes. Dummy tablet locations are now selected only from load-available backends in the current compute group.
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
morrySnow
previously approved these changes
Jul 27, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: the new eligibility predicate correctly restricts dummy locations to load-available, non-decommissioning backends in the current compute group, but two blocking issues remain in the changed path.
Critical checkpoint conclusions:
- Goal and correctness: the main current-group selection goal is implemented and the empty-set path fails explicitly, but single-replica automatic-partition planning can now exhaust its shared candidate list across materialized indexes and fail before execution.
- Scope and reuse: the diff is small and uses the established backend selector predicate. However,
getBackendsByCurrentCluster()has auto-start side effects that are unsafe at the sink call sites' current lock scope. - Concurrency and lifecycle: backend state is sampled without adding shared mutable state or new locks. The blocking issue is that stream, routine, and broker load planning can retain destination-table read locks across a meta-service resume RPC and multi-minute wait.
- Configuration, compatibility, persistence, and protocol: no new configuration, thrift field, storage format, EditLog state, or rolling-upgrade compatibility surface is introduced.
- Parallel paths: insert, stream load, broker load, routine load, group commit, adaptive-bucket handling, cloud/non-cloud selection, and the remote-sink subclass were traced. Group commit bypasses location construction; the remote dummy-location namespace mismatch and virtual-group auto-start ordering are pre-existing rather than distinct regressions in this PR.
- Tests: the added unit test deterministically proves use of the current-group API and exclusion of a load-disabled backend. It does not cover the accepted multi-index/single-replica failure or the auto-start-under-lock path. Per the review-runner contract, no local build or test was run; the live PR currently shows compile and Checkstyle passing while FE UT remains pending.
- Error handling, observability, and performance: unavailable candidates hard-fail and the existing logging is adequate. The unresolved multi-minute metadata-lock hold is the material performance/lifecycle concern.
- User focus: no additional user-provided review focus was supplied; the full PR was reviewed.
Please address both inline findings and add focused coverage before merge.
| SystemInfoService clusterInfo = Env.getCurrentSystemInfo(); | ||
| List<Long> aliveBe = clusterInfo.getAllBackendIds(true); | ||
| if (aliveBe.isEmpty()) { | ||
| List<Long> availableBeIds = clusterInfo.getBackendsByCurrentCluster().values().stream() |
Contributor
There was a problem hiding this comment.
[P1] Avoid auto-start waits while the table read lock is held
getBackendsByCurrentCluster() is not just a snapshot lookup in cloud mode: it calls waitForAutoStart(), which can issue a meta-service RPC and then retry for roughly five minutes. Both StreamLoadHandler.generatePlan and RoutineLoadJob.plan hold the destination table read lock across this sink initialization. If the group transitions out of NORMAL, planning can therefore retain the metadata lock throughout the external wait and block schema changes or other table writers; the old getAllBackendIds(true) call could not do that. Resolve/wake the compute group before taking the table lock, or keep sink initialization on a nonblocking already-resolved backend snapshot.
| int nodeIndex = random.nextInt(nodes.length); | ||
| if (singleReplicaLoad) { | ||
| List<Long> slaveBe = aliveBe; | ||
| List<Long> slaveBe = availableBeIds; |
Contributor
There was a problem hiding this comment.
[P1] Keep backend candidates intact across index iterations
slaveBe aliases availableBeIds, so remove(nodeIndex) permanently shrinks the source used by the next loop iteration. With single-replica load enabled, an empty automatic-partition table with two indexes and one eligible BE in the current compute group handles index 0, then builds an empty nodes array and throws from nextInt(0) for index 1. The current-group filter makes this reachable even when other alive BEs exist outside the group. Keep the removal local to each index and add a multi-index/single-replica test:
Suggested change
| List<Long> slaveBe = availableBeIds; | |
| List<Long> slaveBe = new ArrayList<>(availableBeIds); |
Contributor
TPC-H: Total hot run time: 29577 ms |
Contributor
TPC-DS: Total hot run time: 177482 ms |
Contributor
ClickBench: Total hot run time: 25.23 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Problem Summary: Dummy location planning could wait for cloud compute group auto-start while holding a table read lock. Single-replica planning also mutated the shared backend candidate list across index iterations. Solution: Use a nonblocking compute-group backend snapshot in cloud mode and preserve the existing non-cloud lookup. Copy backend candidates for each single-replica index before removing the primary backend. Tests: - Extend OlapTableSinkTest to verify the auto-starting current-cluster API is not called. - Add single-backend, multi-index single-replica coverage.
github-actions
Bot
removed
the
approved
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: the current-group and per-index candidate fixes are sound for active compute groups, but one blocking cloud auto-start path remains.
Critical checkpoint conclusions:
- Goal and correctness: active-group dummy locations are now restricted to load-available, non-decommissioning backends in the selected compute group, and the per-index candidate list is no longer mutated. The goal is still incomplete because a suspended current group can fail before auto-start when legacy Nereids distribution is selected.
- Scope and reuse: the two-file change is small and uses established backend eligibility predicates. The accepted issue is an ordering gap between compute-group resolution and sink finalization, not unnecessary code expansion.
- Concurrency and locking: no new shared mutable state or lock-order cycle is introduced. Sink finalization still occurs under the destination-table read lock; waking/resolving must happen before that lock, while this helper consumes a side-effect-free snapshot.
- Lifecycle: ConnectContext installation, compute-group propagation, and cleanup were traced across insert and background load paths. No static-initialization, ownership-cycle, or resource-release issue was found.
- Configuration and compatibility: no new configuration, thrift field, FE-BE variable, storage format, symbol, or rolling-upgrade surface is added. The existing supported
enable_nereids_distribute_planner=falsebranch exposes the accepted failure. - Parallel paths: SQL insert, stream load, broker load, routine load, group commit, cloud/non-cloud selection, single-replica loading, multiple indexes, remote sinks, and BE incremental location replacement were reviewed. No additional distinct issue remained after duplicate suppression.
- Tests and results: the added unit tests cover current-group/load-availability filtering and the fixed one-backend/two-index mutation. They do not cover a suspended auto-start group with legacy distribution disabled. Per the review-runner contract, no build or test command was run locally.
- Error handling and observability: empty candidate sets fail explicitly and no new metrics or logging are required. The accepted issue is that a recoverable suspended-group state is converted into that failure too early.
- Transactions, persistence, and data writes: no EditLog, transaction protocol, or persistence format changes are introduced. The accepted failure occurs during planning before dispatch; no separate atomicity or failover defect was found.
- Performance: backend snapshot/filter/copy costs are bounded by backend and index counts. No new hot-path issue distinct from already-known review context was found.
- User focus: no additional user-provided review focus was supplied; the full PR was reviewed.
Please address the inline auto-start finding and add focused coverage before merge.
### What problem does this PR solve? Problem Summary: Switching dummy location creation to a nonblocking backend snapshot avoided waiting under a table read lock, but legacy Nereids distribution could reach sink finalization for a suspended compute group before any auto-start occurred. Wake the selected compute group immediately before acquiring the sink-finalization table lock, while keeping location creation side-effect free. ### Release note None ### Check List (For Author) - Test: Unit Test added; not run locally per request - Behavior changed: Yes. Suspended compute groups are resumed before OLAP sink finalization without holding the table read lock - Does this need documentation: No
sollhui
requested review from
924060929,
englefly and
starocean999
as code owners
July 27, 2026 08:55
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
### What problem does this PR solve? Problem Summary: Newly added dummy-location variables used the older cluster terminology even though they represent the selected compute group's backend set. Rename only those local and test variables while retaining existing compatibility APIs. ### Release note None ### Check List (For Author) - Test: Not run locally per request; naming-only change - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: the current-compute-group eligibility filter and the per-index candidate copy are sound, but three P1 lifecycle and parallel-path defects remain.
Critical checkpoint conclusions:
- Goal and correctness: dummy locations are now restricted to load-available, non-decommissioning backends in the selected compute group. The goal is incomplete because auto-start failure leaks a registered load job, two load entry points do not pre-wake suspended groups, and the lock-free wait can invalidate the supposedly newest table reference.
- Scope and reuse: the four-file diff is focused and uses established backend predicates. The new helper is placed too narrowly in
InsertIntoTableCommand; the pre-wake/resolved snapshot needs to be shared by every caller of dummy-location planning. - Concurrency and locking: the normal insert wait is outside the final table read lock, but its table lookup precedes the long lock-free interval, allowing drop/recreate to evade the identity check. Virtual active/standby resolution can also still perform failover I/O under existing table-lock scopes; that concern is already covered by the earlier lock-wait thread and is not duplicated here.
- Lifecycle and error handling:
OlapInsertExecutorregisters itsInsertLoadJobbefore the new wait, while the wait remains outside the onlyonFailcleanup scope. A thrown wait therefore leaves a non-expiringPENDINGjob. - Configuration and compatibility: no new configuration, thrift field, storage format, EditLog state, symbol, or rolling-upgrade surface is introduced. The supported legacy-distribution and cloud auto-start branches expose the accepted defects.
- Parallel paths: normal insert, transaction-mode values insert, stream load, broker load, routine load, group commit, remote sinks, cloud/non-cloud selection, logical/virtual groups, single-replica loading, and multiple indexes were traced. Transactional values and cloud broker load are the two nonduplicate uncovered callers; stream/routine and virtual failover concerns are already represented by existing review context, while group-commit sinks bypass this dummy-location branch.
- Tests and results: the added unit tests cover current-group/load-availability filtering, per-index candidate isolation, and the helper happy path. They do not cover wait failure cleanup, suspended transaction/broker callers, drop/recreate ordering, or virtual failover I/O. Per the review-runner contract, no local build or test command was run.
- Transactions, persistence, and data writes: no transaction protocol or persistent-format change is added. The defects occur during planning, before or around transaction initialization, and can leave load metadata behind or plan against a removed table ID.
- Performance and observability: backend filtering/copy costs are bounded and no new metric is required. The remaining lock-held virtual failover I/O is already covered by the existing thread; leaked pending load jobs are the new observability and memory concern.
- User focus: no additional user-provided focus was supplied; the full PR was reviewed.
Please address all three inline findings and add focused regression coverage before merge.
| }); | ||
| } | ||
|
|
||
| // lock after plan and check does table's schema changed to ensure we lock table order by id. |
Contributor
There was a problem hiding this comment.
[P1] Re-resolve the target after the auto-start wait
newestTargetTableIf is captured before a wait that can last roughly five minutes and intentionally holds no metadata lock. If the table is dropped and recreated under the same qualified name during that wait, both targetTableIf and this variable still reference the detached old object, so the later ID and schema comparisons pass old-vs-old and sink finalization starts with the removed table ID instead of retrying against the replacement. Perform the wait before this final lookup, or re-fetch by name after it and lock that newly resolved object; a latch-based drop/recreate test would cover the ordering.
| } | ||
|
|
||
| // lock after plan and check does table's schema changed to ensure we lock table order by id. | ||
| TableIf newestTargetTableIf = getTargetTableIf(ctx, qualifiedTargetTableName); |
Contributor
There was a problem hiding this comment.
[P1] Clean up the registered load job when auto-start fails
For a non-inline INSERT ... SELECT with legacy distribution, the OlapInsertExecutor has already registered its InsertLoadJob before this call. If waitForAutoStart() throws (for example on manual shutdown, an RPC error, or resume timeout), execution never enters the following try/catch, so onFail() and recordFinishedLoadJob() are skipped. The job remains PENDING, and incomplete load jobs never expire, so retries accumulate permanent SHOW LOAD entries and memory. Include this wait in the executor cleanup scope (or perform it before registration), and cover the throwing path.
Contributor
TPC-H: Total hot run time: 29740 ms |
Contributor
TPC-DS: Total hot run time: 177612 ms |
Contributor
ClickBench: Total hot run time: 25.04 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Related PR: apache#66083 Problem Summary: Dummy-location planning could keep a stale target table after a long compute-group auto-start wait, leak a registered insert load job when that wait failed, and reach sink planning before auto-start in transactional VALUES and cloud broker load paths. Re-resolve the target after the wait, clean up failed insert executors, and share the pre-planning auto-start step across all affected callers. ### Release note None ### Check List (For Author) - Test: Unit tests added for target re-resolution, wait-failure cleanup, transactional VALUES, and cloud broker load; not run locally per request - Behavior changed: Yes. Suspended compute groups are resumed before dummy-location planning in all affected load paths - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
### What problem does this PR solve? Related PR: apache#66083 Problem Summary: Transactional VALUES insert looked up the target table immediately before the compute-group auto-start wait and then discarded that result. Remove the redundant lookup and resolve the target only after the wait. ### Release note None ### Check List (For Author) - Test: Not run locally per request; covered by the existing batch insert auto-start unit test - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 2nd, 2026 1:27 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 29540 ms |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 2nd, 2026 1:27 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-DS: Total hot run time: 178194 ms |
Contributor
ClickBench: Total hot run time: 25.05 s |
### What problem does this PR solve? Related PR: apache#66083 Problem Summary: Dummy-location planning only needs a snapshot of load-available backends in the current compute group. Remove the pre-planning compute-group auto-start waits and their lifecycle-specific tests, while retaining the current-group snapshot and load-availability filtering in OlapTableSink. ### Release note None ### Check List (For Author) - Test: Not run locally per request; existing OlapTableSink unit tests cover current compute group selection and load-availability filtering - Behavior changed: Yes. Dummy-location planning no longer initiates compute-group auto-start - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29609 ms |
Contributor
TPC-DS: Total hot run time: 177289 ms |
Contributor
ClickBench: Total hot run time: 25.12 s |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 2nd, 2026 1:27 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: the current head correctly restricts dummy locations to load-available, non-decommissioning backends in the selected compute group, and the per-index candidate copy fixes the earlier single-replica list mutation. It is not merge-ready because the already-filed compute-group lifecycle and lock-boundary issues remain. I found no additional nonduplicate inline issue, so this review intentionally adds no duplicate inline comments.
Critical checkpoint conclusions
- Goal and proof: The added unit tests prove the awake-group selection and multi-index candidate-copy behavior. They do not prove the end-to-end requirement for a suspended group, where sink initialization can consume an empty nonblocking snapshot before any caller wakes the group.
- Scope and clarity: The two-file change is small and focused, and the eligibility predicates match established load selection. The local implementation is clear, but the behavior needs a shared caller-level lifecycle fix.
- Concurrency and locks: Backend lists are copied and the eligibility state is atomically read. The five-minute auto-start wait is gone from this helper, but a virtual compute group can still take
getBackendsByClusterName()throughgetPhysicalCluster()→switchActiveStandby()→MetaServiceProxy.alterCluster()while sink callers hold the table read lock. This is covered by the existing lock-held external-work thread. - Lifecycle and parallel paths: Normal insert with legacy distribution can still reach the snapshot before auto-start, as covered by the existing normal-insert thread. Transactional VALUES and cloud broker load retain the same pre-wake gap covered by the existing parallel-caller thread. Group commit intentionally bypasses location initialization; no other distinct caller was found.
- Configuration: No configuration item or dynamic-reload behavior is added.
- Compatibility: No function symbol, storage format, persisted state, or FE/BE thrift field changes. Existing V1/V2 fake-tablet and real-location replacement behavior remains intact.
- Conditions and error handling: Missing context and empty candidates fail explicitly. Production insert/load call chains establish the correct thread-local context and selected group; no separate stale/wrong-context bug was substantiated.
- Tests and results: The new tests are isolated and cover the two local fixes. Live
FE UT, compile, CheckStyle, and cloud regression checks pass. The live FE coverage check is failing, but its TeamCity details were not accessible. No local build or tests were run because this review runner's contract forbids them. - Observability: No new critical state requires additional metrics; existing errors/logging cover the local selection failure, though the lifecycle failure itself must still be fixed.
- Transactions, persistence, and data writes: This patch changes planning only and introduces no journal or transaction-state format. The current head removed the temporary auto-start wait that caused the earlier stale-table/load-job cleanup defects, so those particular older threads are not new blockers on this head.
- FE/BE variable propagation: No new cross-process variable is introduced.
- Performance: The ordinary physical-group path uses a copied cached snapshot and introduces no material hot-path regression. Virtual-group failover is the lock-held external-I/O exception noted above.
- Other issues: The previous cross-index aliasing issue is fixed. No additional correctness, memory-safety, security, or rolling-upgrade issue was substantiated.
No additional user review focus was provided. One complete convergence round was performed: both normal full-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS against the same current ledger and live existing-comment set.
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
4 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?
Automatic partition loads with no physical partitions create dummy tablet locations. The previous logic sampled from all alive backends in a cloud instance, allowing a backend from another compute group to be selected. Restrict candidates to load-available, non-decommissioning backends in the current compute group before choosing a dummy location.
Release note
Fix automatic partition loads to choose dummy tablet locations from the current compute group.
Check List (For Author)