deardeng commented on PR #68119:
URL: https://github.com/apache/doris/pull/68119#issuecomment-5713455480
The atomic flip is the right fix, but the rewrite also changes the request
contract of `DECOMMISSION_NODE` / `NOTIFY_DECOMMISSIONED`, which are exposed
over HTTP (`/MetaService/http/decommission_node`, `notify_decommissioned`) and
called by external ops tooling. Three behavior changes vs. the current code:
1. **`cluster_id` is now required.** The old code never read
`request->cluster().cluster_id()` at all — it resolved the node via
`get_node(nodes[0].cloud_unique_id)` and took `cluster_id` from the `NodeInfo`
in the index. After this PR, `update_cluster()` rejects the request with
`"missing cluster_id"`. Any caller that only sends `cloud_unique_id + ip +
heartbeat_port` (which was the documented-enough contract so far) breaks.
2. **A wrong/stale `cluster_id` is now fatal or, worse, silently ignored.**
Old: ignored, node found by endpoint, status flipped, OK. New: if the id
doesn't exist → `"failed to find cluster to update"`; if it exists but the node
isn't in it → `flip_nodes_status_in_place` only logs a WARNING, returns `""`,
`update_cluster` commits (mtime bump only), and the caller gets `OK` with
nothing changed.
3. **Zero matched nodes now returns OK.** Old code fed an empty `to_del`
into `modify_nodes`, which rejected it (`"to_add and to_del both empty or both
not empty"` → `INVALID_ARGUMENT`). New code returns OK after a WARNING. Callers
(and the FE decommission loop) lose the signal that the node wasn't found.
Suggestion — keep the node-resolution path exactly as before and only
replace the delete+re-add with the in-place flip:
- Resolve targets via `get_node(nodes[0].cloud_unique_id)` + endpoint match
(as today), group by the `cluster_id` from `NodeInfo`, and do **not** consult
the request's `cluster_id` (it was never consulted).
- One `update_cluster(instance_id, {cluster_id}, filter-by-that-id, flip)`
per resolved cluster.
- Inside the flip, match by `cloud_unique_id + endpoint` (the same key
`modify_to_del` used) and return a non-empty msg if any target is missing, so
the txn aborts and the caller gets an error as before.
- Zero resolved targets → return an error, not a WARNING.
The existing `DecommissionNodeFlipStatusInPlaceTest` still passes with that
approach (it calls `refresh_instance` after seeding KV). Please also add: (a) a
request without `cluster_id` → `OK`; (b) an endpoint that matches nothing →
non-OK.
--
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]
