sodonnel commented on code in PR #9836:
URL: https://github.com/apache/ozone/pull/9836#discussion_r2877912989
##########
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:
I'm not sure. The finalize thread still needs to query the nodes, and the
heartbeat can update them concurrently. I would be happier to leave
synchronized in place, as I doubt it causes much performance impact (at least
nothing that has been noted so far) and if there is some race condition caused
by removing it, it could be a long time before we find it. We will likely have
long forgotten about this change by then!
--
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]
