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: [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]
