errose28 commented on code in PR #9836:
URL: https://github.com/apache/ozone/pull/9836#discussion_r2875339726
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeStateManager.java:
##########
@@ -847,14 +764,6 @@ public synchronized void checkNodesHealth() {
updateNodeState(node, staleNodeCondition, status,
NodeLifeCycleEvent.TIMEOUT);
break;
- case HEALTHY_READONLY:
- updateNodeLayoutVersionState(node, layoutMatchCondition, status,
- NodeLifeCycleEvent.LAYOUT_MATCH);
Review Comment:
I think we need to look at this area a little more. `LAYOUT_MATCH` and
`LAYOUT_MISMATCH` are removed from the state machine `nodeHealthSM`, so we will
get a warning logged if we try to use them in `updateNodeLayoutVersionState`
like the previous case block is doing. It looks like we can delete these from
the `NodeLifeCycleEvent` enum and remove `updateNodeLayoutVersionState`, since
it now has a warning that the `lifeCycleEvent` parameter is always
`LAYOUT_MISMATCH`.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java:
##########
@@ -268,40 +265,6 @@ public void testGetLastHeartbeatTimeDiff() throws
Exception {
}
}
- /**
- * Tests that node manager handles layout version changes from heartbeats
Review Comment:
We should still be sending finalize commands to nodes that are in lower
layout versions. Is there still test coverage for this?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeStateManager.java:
##########
@@ -755,41 +704,9 @@ public void run() {
scheduleNextHealthCheck();
}
- /**
- * Upgrade finalization needs to move all nodes to a healthy readonly state
- * when finalization finishes to make sure no nodes with metadata layout
- * version older than SCM's are used in pipelines. Pipeline creation is
- * still frozen at this point in the finalization flow.
- *
- * This method is synchronized to coordinate node state updates between
- * the upgrade finalization thread which calls this method, and the
- * node health processing thread that calls {@link #checkNodesHealth}.
- */
- public synchronized void forceNodesToHealthyReadOnly() {
- try {
- List<DatanodeInfo> nodes = nodeStateMap.getDatanodeInfos(null, HEALTHY);
- for (DatanodeInfo node : nodes) {
- nodeStateMap.updateNodeHealthState(node.getID(),
- HEALTHY_READONLY);
- if (state2EventMap.containsKey(HEALTHY_READONLY)) {
- // At this point pipeline creation is already frozen and the node's
- // state has been updated in nodeStateMap. This event should be a
- // no-op aside from logging a message, so it is ok to complete
- // asynchronously.
- eventPublisher.fireEvent(state2EventMap.get(HEALTHY_READONLY),
- node);
- }
- }
-
- } catch (NodeNotFoundException ex) {
- LOG.error("Inconsistent NodeStateMap! {}", nodeStateMap);
- }
- }
-
/**
* This method is synchronized to coordinate node state updates between
- * the upgrade finalization thread which calls
- * {@link #forceNodesToHealthyReadOnly}, and the node health processing
+ * the upgrade finalization thread and the node health processing
Review Comment:
Is the synchronization still required here after we removed
`forceNodesToHealthyReadOnly`?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/upgrade/TestHddsUpgradeUtils.java:
##########
@@ -204,15 +195,13 @@ public static void
testDataNodesStateOnSCM(List<StorageContainerManager> scms,
* setting "alternateState = null".
*/
public static void testDataNodesStateOnSCM(StorageContainerManager scm,
- int expectedDatanodeCount, HddsProtos.NodeState state,
- HddsProtos.NodeState alternateState) {
+ int expectedDatanodeCount, HddsProtos.NodeState state) {
int countNodes = 0;
for (DatanodeDetails dn : scm.getScmNodeManager().getAllNodes()) {
try {
HddsProtos.NodeState dnState =
scm.getScmNodeManager().getNodeStatus(dn).getHealth();
- assertTrue((dnState == state) ||
- (alternateState != null && dnState == alternateState));
+ assertSame(dnState, state);
Review Comment:
nit. assert(expected, actual)
```suggestion
assertSame(state, dnState);
```
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java:
##########
Review Comment:
This comment is also stale.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java:
##########
Review Comment:
This comment is stale.
--
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]
