Conversation
…ected new_cluster_id but was null; current-group removal passes) - Behavior changed: No - Does this need documentation: No [fix](cloud) Preserve recreated compute group mapping Issue Number: None Related PR: None Problem Summary: Recreating a compute group with the same name and a new ID updates the name mapping to the new ID. Removing the obsolete group then unconditionally removed that name, leaving the new backends and compute group unreachable by name. Delete the mapping only when it still points to the group being removed, and remove duplicate cleanup from the backend deletion path. Fix compute group name resolution after recreating a group with the same name and a new ID. - Test: Unit Test (targeted CloudSystemInfoService tests, 2 passed) - Behavior changed: Yes (obsolete group cleanup preserves a same-name replacement mapping) - 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 |
![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: three P1 correctness gaps remain in compute-group name ownership reconciliation.
- The production physical checker adds the same-name replacement before deleting the obsolete ID, so its reverse name lookup can return empty and permanently skip old-ID cleanup.
- Both rename helpers still remove the old name without checking which ID owns it, so rename followed by reuse of the freed name can erase the replacement mapping.
- The independently fetched physical and instance-checker snapshots have no generation ordering or live repair pass; a stale cross-type add can overwrite the current owner, whose subsequent removal leaves the current group nameless.
Critical checkpoint conclusions:
- Goal and proof:
remove(name, expectedId)is correct once reached, but the stated production recreation lifecycle is not fixed end to end. The new tests exercise direct helpers rather than the scheduler/daemon boundaries. - Scope, clarity, and reuse: the patch is small and correctly centralizes duplicate cleanup, but misses the upstream physical deletion gate, the two parallel rename writers, and stale-response ownership.
- Concurrency and lifecycle: map mutations serialize under the static reentrant write lock, and no deadlock or torn mutation was found. The remaining failures are phase/generation ordering bugs across the independent physical and instance daemons. Physical/virtual add, remove, recreate, rename, explicit drop, and backend replay paths were traced.
- Configuration and compatibility: no configuration, RPC/protobuf, FE-BE variable, symbol, persisted storage format, or new edit-log record changes.
- Conditions, errors, and observability: pair removal fails safely, but the physical checker's empty reverse lookup silently prevents cleanup; consistency validation only warns and does not repair lost mappings. No new ignored status or exception-boundary defect was found.
- Tests: add checker-entry recreation coverage, rename-plus-name-reuse coverage, and a latch/barrier stale-snapshot test, each including a later reconciliation cycle and cleanup assertions. The PR author reports the two targeted tests passed; this review was static and did not run builds/tests as required by the review contract.
- Persistence, data, performance, and memory: user-data transactions, MoW, and storage writes are unaffected. The map change remains O(1) with no hot-path regression; the first finding can retain obsolete backend/group resources indefinitely.
- Security: this consumes trusted internal Meta Service state and does not change authentication, RBAC, or tenant-isolation enforcement.
- User focus: no additional focus was provided, so the full PR was reviewed.
- Completion: converged within the three-round cap. All candidates were independently verified and resolved; no duplicate live or bundled inline review was present.
Contributor
TPC-H: Total hot run time: 16867 ms |
Contributor
TPC-DS: Total hot run time: 82489 ms |
Contributor
ClickBench: Total hot run time: 14.7 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
…ation Related PR: apache#67913 Problem Summary: Adding a same-name replacement before deleting an obsolete group makes reverse name lookup fail and leaves old backends behind. Rename cleanup also removes names already owned by another group. Independently fetched physical and virtual snapshots can overwrite a current mapping, and unchanged nodes previously prevented later cycles from repairing it. Clean obsolete groups by ID using their own backend tags, guard both rename removals by the expected ID, and refresh name mappings for locally installed groups on each successful physical or virtual synchronization. This provides eventual repair after stale snapshots without adding snapshot versioning. Add a cloud Docker case with controlled checker pauses for recreation, rename and reuse, and a delayed virtual snapshot followed by periodic repair. Fix stale backend metadata and missing compute group name mappings after same-name recreation, rename and reuse, or delayed cloud metadata synchronization. - Test: Unit Test (GREEN checkpoint) - ./run-fe-ut.sh --run org.apache.doris.cloud.catalog.CloudClusterCheckerTest,org.apache.doris.cloud.catalog.CloudInstanceStatusCheckerTest,org.apache.doris.cloud.system.CloudSystemInfoServiceTest: 36 passed, 0 failures, 0 errors. - Targeted FE Checkstyle: 0 violations; git diff --check passed. - Cloud Docker regression added and Groovy syntax parsed; execution intentionally left to the user. No .out file generated or handwritten. - Behavior changed: Yes, periodic synchronization repairs name mappings and cleans obsolete group IDs even after name reuse. - Does this need documentation: No None
Related PR: apache#67913 Problem Summary: Skip the cloud-only compute group name reconciliation Docker suite before constructing the cluster when the regression environment is not in cloud mode. None - Test: Groovy syntax parsing and git diff --check passed. Docker case not run as requested. - Behavior changed: Yes, skip this regression suite in non-cloud mode. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: two P1 correctness/lifecycle defects and one P2 regression-result provenance gap remain.
Critical checkpoint conclusions:
- Goal and proof: the PR aims to preserve and eventually repair compute-group name ownership across recreation, rename, reuse, and stale daemon snapshots. It fixes the three previously reviewed map failures, but rejected virtual records can still mutate the index (P1), and name-only warm-up cleanup is not owner-safe (P1), so the end-to-end goal is not yet fully met.
- Scope, clarity, and reuse: the production patch is compact and centralizes group cleanup and periodic name repair. The main concerns are localized to the new refresh input and to ownership semantics shared by deletion and the parallel rename path; no unrelated production expansion was found.
- Concurrency:
CloudClusterChecker,CloudInstanceStatusChecker, and user command threads can interleave. The compute-group maps are serialized byCloudSystemInfoService's read/write lock, with lightweight map work under the lock and no new lock-order or deadlock issue found. The accepted failures arise between separately locked phases: a replacement can become visible before old-owner cleanup, and independently fetched snapshots can be applied in either order. - Lifecycle: physical/virtual add, rename, removal, same-name reuse, stale/current cycles, master/follower behavior, placement cleanup, metrics, and persisted warm-up-job states were traced. No static-initialization or reference-lifetime issue applies. P1 shows that orphaned name-scoped jobs can persist across group removal, while replacement-owned jobs can be irreversibly cancelled.
- Configuration and compatibility: no configuration, RPC/protobuf, FE-BE variable, function symbol, edit-log schema, or storage-format change is introduced, so no rolling-upgrade compatibility work is required. Debug points are inert unless enabled and use the existing timeout mechanism.
- Parallel paths and conditions: physical and virtual rename/removal plus current and rejected refresh paths were compared. Expected-ID map removal is correct for the mapping itself, but the same transient name index is insufficient to establish warm-up-job ownership; validation rejection is also bypassed by the unconditional virtual refresh.
- Tests and results: the new unit tests cover checker-entry recreation, rename/reuse, a stale-snapshot barrier, and later-cycle repair. They do not cover rejected virtual rename/removal or either warm-up-job interleaving. The author reports 36 targeted FE tests passing, but this review was static and ran no builds/tests under the review contract. The cloud Docker suite was reported as not run, and its
.outwas reported as not generated, producing the P2 provenance finding. Current unrelated FE-UT failures do not establish a changed-path defect. - Errors and observability: existing logs carry group IDs/names, and the added debug-point phase logs are adequate for the regression orchestration. No new ignored status or exception-boundary defect was found. The P1 validation issue is a silent state mutation after an explicit rejection path.
- Persistence, transactions, and data: no user-data transaction, visible-version, MoW, or storage-write path changes. Compute-group map mutations remain atomic under the service lock. Warm-up jobs persist source/destination names without group IDs, which is the root of the accepted lifecycle issue.
- Performance and memory: refresh is linear in the returned group collection and performs only map checks/updates under the lock; no material CPU, allocation, FE/BE memory-accounting, or hot-path regression was found.
- User focus: no additional focus was provided, so the complete eight-file PR scope was reviewed.
- Completion: normal production, normal test/coverage, and separate risk-focused reviews converged in round 2. Every candidate was verified, merged, duplicate-fenced, or dismissed with code evidence; no suspicious point remains unresolved.
### What problem does this PR solve? Related PR: apache#67913 Problem Summary: A virtual-group rename with an invalid policy or subgroup shape is rejected by metadata reconciliation, but the raw snapshot previously still published its name. Removing that group later only removed its locally recorded name, leaving aliases pointing to a missing ID. Return reconciliation success for new and existing virtual groups and refresh names only for accepted records. Keep the complete snapshot for obsolete-group detection, and remove all name mappings owned by a removed ID under the existing write lock while preserving reused names. Add tests for rejected policy and subgroup updates, accepted renames, rejected new groups, and stale-alias cleanup. Explicitly set the valid policy type in test snapshots. Warm-up job ownership by compute-group ID remains out of scope. ### Release note Prevent rejected virtual-group updates from publishing invalid name mappings and clean up stale aliases when compute groups are removed. ### Check List (For Author) - Test: Unit Test: bash run-fe-ut.sh --run org.apache.doris.cloud.catalog.CloudClusterCheckerTest,org.apache.doris.cloud.catalog.CloudInstanceStatusCheckerTest,org.apache.doris.cloud.system.CloudSystemInfoServiceTest; 41 tests passed, 0 failures, 0 errors, 0 skipped. Before the fix, CloudClusterCheckerTest reproduced 3 intended assertion failures out of 11 tests. FE Checkstyle passed with 0 violations; git diff --check passed. Tests were not rerun for this history-only squash; the resulting tree is unchanged. Docker regression was not run, as requested; no regression output was changed. - Behavior changed: Yes (rejected virtual updates no longer publish names or proceed to cache-task synchronization; deletion clears all aliases owned by the removed ID) - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16788 ms |
Contributor
TPC-DS: Total hot run time: 83307 ms |
Contributor
ClickBench: Total hot run time: 14.78 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
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.
Problem Summary: Recreating a compute group with the same name and a new ID updates the name mapping to the new ID. Removing the obsolete group then unconditionally removed that name, leaving the new backends and compute group unreachable by name. Delete the mapping only when it still points to the group being removed, and remove duplicate cleanup from the backend deletion path.
Fix compute group name resolution after recreating a group with the same name and a new ID.
Test: Unit Test (targeted CloudSystemInfoService tests, 2 passed)
Behavior changed: Yes (obsolete group cleanup preserves a same-name replacement mapping)
Does this need documentation: No
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)