deardeng opened a new pull request, #70224: URL: https://github.com/apache/doris/pull/70224
### Background In cloud mode, a BE's rowset RPC carries its cloud_unique_id, and MS resolves the owning instance through the registered node information. A decommission state change must keep that registration available while in-flight loads finish. ### What problem does this PR solve? Issue Number: None Related PR: None PR Type: Bugfix Problem Summary: Both start-decommission and completion updated node status by deleting the node and re-adding it in separate transactions. Each transaction independently committed and refreshed the MS cache, exposing a state with the target node missing. A concurrent rowset commit could then fail with "failed to commit rowset: empty instance_id". Update the existing node's status in one transaction and add deterministic regression coverage for both transitions. ### Details - **Change map:** 188 production lines changed, including the state-update fix and injection support; 299 test-code lines changed; 7 generated expected-output lines. The two handlers are consolidated, and most additions are tests. - **Frequency:** Intermittent during concurrent loading and decommission state changes. The new regression fixes the execution order and reproduces the old behavior for both transitions. - **Root cause:** The delete transaction publishes metadata and a cache snapshot without the node before the add transaction restores it. If that removes the last mapping for the BE's cloud_unique_id and the ID cannot be decoded through the compatibility format, get_instance_id returns empty. The cache lock protects each refresh, but does not make the two transactions atomic. - **Why existing tests missed it:** The regression issued INSERTs once per second without controlling whether a rowset commit hit this short window. Existing HTTP coverage used cache-only node setup and checked the final response. - **Fix:** Share a handler that uses ResourceManager::update_cluster to match existing nodes by UID and endpoint within the target cluster, change only their status, commit once, and refresh once. Preserve node identity, timestamps, and node-based HTTP cluster lookup. An unmatched batch returns an error before any partial change is published. - **Other paths and branches:** Apply the same handler to DECOMMISSION_NODE and NOTIFY_DECOMMISSIONED. This PR targets master; no backports are included. - **Behavior change:** Status changes retain node registration throughout the update. Requests containing missing or mismatched nodes fail without persisting part of the batch. ### Testing The regression uses one BE and one bucket so the load must reach the target node: 1. Start a real INSERT and hold its commit_rowset RPC immediately before instance lookup. 2. Run the status update and hold it after its transaction has committed and refreshed the cache. 3. Confirm both gates were reached, then release only the rowset RPC. Require INSERT to succeed and COUNT(*) / SUM(id) to both equal 1 while the state-update request is still held. 4. Clear the gates in finally. For the completion case, first put the node into DECOMMISSIONING while the rowset commit is held. The injection points control execution order; the node-state implementation determines which metadata is published. | Implementation under test | Result | | --- | --- | | Fixed start and completion handlers | Full cloud_decommission passed, including both controlled interleavings and the original 2-BE/3-BE, WAL, continuous INSERT, and FE-restart scenarios | | Old start and completion handlers | The controlled start case failed with failed to commit rowset: empty instance_id | | Fixed start handler and old completion handler | The start case and its result check passed; the controlled completion case failed with the same empty instance_id error | Negative runs used isolated suite identifiers to run beside the positive cluster. SQL, assertions, expected data, and gates were identical; the node-state handlers were switched between fixed and old implementations. - Four new deterministic unit tests fail with the old implementation and pass with the fix. - resource_test: 19 tests passed under ASAN. - MetaServiceHttpTest: 32 tests passed under ASAN, using the existing repository LeakSanitizer suppressions. - The expected .out file was generated by the regression runner and compared in the final normal positive run. - Repository clang-format 16, build-hygiene, and clang-tidy checks on changed lines passed. ### Notes for Reviewers MS was built from this source with ASAN and injection points enabled; FE/BE reused an existing test image. No RPC fields or stored metadata formats change. Non-injection builds compile the new checkpoints away. ### Release note Fix intermittent empty instance_id load errors during cloud backend decommissioning. ### Check List (For Author) - Test: Regression test, unit tests, and static checks described above - Behavior changed: Yes (see Details) - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Codex - Generated: Code, tests, and this description - Human review: Pending author review -- 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]
