adoroszlai commented on code in PR #4140:
URL: https://github.com/apache/ozone/pull/4140#discussion_r1114254142


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -1894,6 +1897,26 @@ public void updatePeerList(List<String> newPeers) {
         }
       }
     }
+    RaftPeer leader = null;
+    try {
+      leader = omRatisServer.getLeader();
+    } catch (IOException ex) {
+      LOG.error("IOException while getting the " +
+          "Ratis server leader.", ex);
+    }
+    if (Objects.nonNull(leader)) {
+      String leaderId = leader.getId().toString();
+
+      // If leaderId is empty, then leader is undefined
+      // and current OM is neither leader nor follower.
+      // OMHAMetrics shouldn't be registered in that case.
+      if (!Strings.isNullOrEmpty(leaderId)) {

Review Comment:
   @neils-dev @xBis7
   
   I had to update this part of the code after merging the PR due to a 
conflicting change that had been just merged (HDDS-6743, `getLeader()` no 
longer throws `IOException`).
   
   Upon closer inspection, I think these `if-else` blocks have a problem.  
Please correct me if I'm wrong, but if leader is undefined then `leader` will 
be `null`, so the unregistration will not happen.  (This seems to have been the 
case even before HDDS-6743.)  If we have any leader information, its `id` 
cannot be `null`.
   
   I think it should be:
   
   ```java
   if (Objects.nonNull(leader)) {
     // init
   } else {
     // unregister
   }
   ```



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

Reply via email to