You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "yandrey321 (via GitHub)" <gi...@apache.org> on 2026/09/28 19:59:19 UTC
[PR] HDDS-16630. Update the network topology when a datanode re-registers … [ozone]
yandrey321 opened a new pull request, #11353:
URL: https://github.com/apache/ozone/pull/11353
## What changes were proposed in this pull request?
`SCMNodeManager.register()` handles the re-registration of an already known datanode in three
branches, but only one of them keeps the network topology in step with the node state:
| re-registration | `clusterMap.update` | `nodeStateManager.updateNode` |
| --------------- | ------------------- | ----------------------------- |
| ip or hostname changed | yes | yes |
| version changed | **no** | yes |
| port set changed | **no** | yes |
The order in the first branch is load-bearing. `NetworkTopologyImpl.update` ends in
`clusterTree.add(newNode)`, which sets the parent on the new node object, and
`NodeStateManager.updateNode` then builds the stored `DatanodeInfo` from that same object, whose copy
constructor carries the parent across. A branch that updates the node state alone therefore leaves
three problems behind:
1. The topology keeps the stale record, with the old port set and the old version. Anything that reads
a datanode back out of the topology — placement, topology-aware read ordering — sees the ports the
datanode no longer exposes.
2. The freshly stored `DatanodeInfo` has no parent, so `NetworkTopologyImpl.containsNode` walks to
`null` and `clusterMap.contains(nodeManager.getNode(id))` returns false for a node that is,
as far as SCM is concerned, registered and healthy.
3. A datanode that `DeadNodeHandler` had already removed from the topology never re-enters it. This
one does not heal. `NodeStateManager.updateNode` recomputes the status straight to `HEALTHY` and
`nodeStateMap.updateNode` publishes no state-change event, so the DEAD -> HEALTHY_READONLY
transition that `HealthyReadOnlyNodeHandler` re-adds the node on never happens. A datanode that
restarts with a new version or a new port set after being marked dead stays absent from the
topology for the lifetime of the SCM.
Both missing branches are reachable in normal operation: the version branch on any datanode upgrade,
and the port branch whenever a datanode restarts with a different port set, for example when Ratis
DataStream is enabled and `RATIS_DATASTREAM` appears.
### Approach
The body of the first branch already does the right thing in the right order, so this patch extracts
it into a private `updateNodeAndTopology(oldNode, datanodeDetails, layoutInfo)` and calls it from all
three branches, rather than duplicating `clusterMap.update` plus the parent assertion twice more. The
helper's javadoc records why the topology has to be updated before the node state, so the ordering is
not silently reversed later.
Re-entry into the topology needs no special case. When `oldNode` is absent from the tree,
`containsNode(oldNode)` is false, `NetworkTopologyImpl.update` skips the removal, and
`clusterTree.add(newNode)` inserts the node again.
Couple things to consider:
* A DEAD datanode that re-registers with nothing at all changed still fires no branch, so this patch
does not add it back to the topology on registration. That case does heal on its own: the next
heartbeat drives DEAD -> HEALTHY_READONLY and `HealthyReadOnlyNodeHandler` re-adds the node.
Forcing a topology write on every no-op re-registration would be wider than the bug.
* `NetworkTopologyImpl.update` throws `InvalidTopologyException` (a `RuntimeException`) on a
network-location depth mismatch, and `register` only catches `NodeNotFoundException`. The version
and port branches now inherit that exposure from the ip/hostname branch. It appears unreachable,
because `clusterMap.add` on the initial-registration path applies the same depth check, so a node
with a mismatched depth could not have registered in the first place.
Generated-by: Claude Code (Claude Opus 5)
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16630
## How was this patch tested?
Unit tests in `TestSCMNodeManager`, extending the existing suite. `portsChanged` had no
`SCMNodeManager`-level coverage before this patch; it was only unit-tested in `TestDatanodeDetails`.
* `testRegisterRefreshesPortsOnPortChange` (added in HDDS-15799) gains topology assertions: the leaf
count, `clusterMap.contains(stored)`, and that the object held *in* the topology exposes the new
`RATIS_DATASTREAM` port rather than being the stale one.
* `testRegisterUpdatesTopologyOnVersionChange` is new. It covers a version-only re-registration, then
removes the node from the topology the way `DeadNodeHandler` does for a dead node and asserts that
re-registration puts it back.
Results:
* `TestSCMNodeManager` 51/51 and `TestDeadNodeHandler` 4/4 — 55 tests, 0 failures.
* Negative control: with only the version and port branches reverted to the previous
`nodeStateManager.updateNode` call, both tests fail with
`refreshed node should still be in the network topology ==> expected: <true> but was: <false>`.
Line coverage alone would not have caught this regression, since the old call sites were covered
too, so the assertions were verified to actually bite.
* JaCoCo on the new code: `updateNodeAndTopology` is `LINE 5/5`, `INSTRUCTION 23/24`, `BRANCH 1/2`,
and all three call sites are executed. The single uncovered instruction and branch is the throwing
side of `Preconditions.checkState(dn.getParent() != null)`, an invariant assertion that is
unreachable unless `NetworkTopology.update` returns without parenting the node. The ip/hostname
branch this was lifted from has the same uncovered throw.
* `mvn clean install -DskipTests -DskipShade -DskipRecon -DskipDocs` — BUILD SUCCESS.
* `./hadoop-ozone/dev-support/checks/checkstyle.sh` — 0 violations across 58 modules.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16630. Update the network topology when a datanode re-registers with new ports or a new version [ozone]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #11353:
URL: https://github.com/apache/ozone/pull/11353#discussion_r4138918019
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -506,6 +503,25 @@ public RegisteredCommand register(
.build();
}
+ /**
+ * Replaces the record of an already registered datanode, in both the node state and the network
+ * topology. The topology has to be updated first: {@link NetworkTopology#update} sets the parent on
+ * the new node, and {@link NodeStateManager#updateNode} only picks it up because the stored
+ * {@link DatanodeInfo} is copied from that node afterwards. Updating the node state alone would
+ * leave the stale record in the topology and the stored node without a parent.
+ *
+ * @return the refreshed node as held by the node state manager
+ */
+ private DatanodeDetails updateNodeAndTopology(DatanodeInfo oldNode,
+ DatanodeDetails datanodeDetails, LayoutVersionProto layoutInfo)
+ throws NodeNotFoundException {
+ clusterMap.update(oldNode, datanodeDetails);
+ nodeStateManager.updateNode(datanodeDetails, layoutInfo);
Review Comment:
A concurrent `DeadNodeHandler` can still undo this repair. It can observe `DEAD` and make `contains(oldNode)` return true, then this update replaces the topology entry, after which the handler's separate `remove(oldNode)` removes the newly added entry by path. `updateNode` then publishes `HEALTHY` without a transition event, and the parent assertion still passes because removal clears the old object’s parent, not `datanodeDetails`’ parent. The node is therefore again healthy but permanently absent from topology. Please coordinate registration with dead-node topology removal under a shared atomic protocol/lock (and add the corresponding interleaving test) rather than relying on update-before-state ordering alone.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16630. Update the network topology when a datanode re-registers with new ports or a new version [ozone]
Posted by "yandrey321 (via GitHub)" <gi...@apache.org>.
yandrey321 commented on PR #11353:
URL: https://github.com/apache/ozone/pull/11353#issuecomment-5902069435
Merged the master to this branch.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16630. Update the network topology when a datanode re-registers with new ports or a new version [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #11353:
URL: https://github.com/apache/ozone/pull/11353#issuecomment-5902035146
> The concurrency concern in [(earlier comment)](https://github.com/apache/ozone/pull/11353#discussion_r4138918019) is valid. I implemented a follow-up fix on top of this PR's current head ([52c5d06](https://github.com/apache/ozone/commit/52c5d06b6cb95822cffcbef1907cf35ec43357aa)): [jojochuang@293f9f8](https://github.com/jojochuang/ozone/commit/293f9f80ee1ea0b3aaa44fcd81bb57efd0dbd837)
@jojochuang, looks like this has been in https://github.com/apache/ozone/pull/11343
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16630. Update the network topology when a datanode re-registers with new ports or a new version [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #11353:
URL: https://github.com/apache/ozone/pull/11353#issuecomment-5904694490
The data race discovered by Copilot looks legit. It's an existing issue, can be addressed separately.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16630. Update the network topology when a datanode re-registers with new ports or a new version [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #11353:
URL: https://github.com/apache/ozone/pull/11353#issuecomment-5902050841
@yandrey321 could you please merge upstream/master so we can reflect the latest fixes for SCM? Thanks!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16630. Update the network topology when a datanode re-registers with new ports or a new version [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang merged PR #11353:
URL: https://github.com/apache/ozone/pull/11353
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16630. Update the network topology when a datanode re-registers with new ports or a new version [ozone]
Posted by "yandrey321 (via GitHub)" <gi...@apache.org>.
yandrey321 commented on PR #11353:
URL: https://github.com/apache/ozone/pull/11353#issuecomment-5914613155
> The data race discovered by Copilot looks legit. It's an existing issue, can be addressed separately.
I can open separate JIRA for the race condition and fix it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16630. Update the network topology when a datanode re-registers with new ports or a new version [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #11353:
URL: https://github.com/apache/ozone/pull/11353#issuecomment-5915154459
Merged. Thanks @yandrey321 @peterxcli @rich7420
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org