[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2167: HDDS-5122. SCM Reinitialization can end up leaking Ratis Segmented RaftLogWorker threads

[GitBox <gi...@apache.org>]

bharatviswa504 commented on a change in pull request #2167:
URL: https://github.com/apache/ozone/pull/2167#discussion_r617266590



##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -937,13 +943,20 @@ public static boolean scmInit(OzoneConfiguration conf,
       }
     } else {
       clusterId = scmStorageConfig.getClusterID();
-      LOG.info("SCM already initialized. Reusing existing cluster id for sd={}"
-              + ";cid={};layoutVersion={}", scmStorageConfig.getStorageDir(),
-          clusterId, scmStorageConfig.getLayoutVersion());
-      if (SCMHAUtils.isSCMHAEnabled(conf)) {
-        SCMRatisServerImpl.reinitialize(clusterId, scmStorageConfig.getScmId(),
-            haDetails.getLocalNodeDetails(), conf);
+      final boolean isSCMHAEnabled = scmStorageConfig.isSCMHAEnabled();
+      if (SCMHAUtils.isSCMHAEnabled(conf) && !isSCMHAEnabled) {
+        SCMRatisServerImpl.initialize(scmStorageConfig.getClusterID(),
+            scmStorageConfig.getScmId(), haDetails.getLocalNodeDetails(),
+            conf);
+        scmStorageConfig.setSCMHAFlag(true);
+        scmStorageConfig.setPrimaryScmNodeId(scmStorageConfig.getScmId());

Review comment:
        We set primaryScmID irrespective of HA scmStorageConfig.setPrimaryScmNodeId(scmStorageConfig.getScmId());

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMStorageConfig.java
##########
@@ -60,6 +57,13 @@ public void setScmId(String scmId) throws IOException {
     }
   }
 
+  public void setSCMHAFlag(boolean flag) {
+    if (!isSCMHAEnabled()) {
+      Preconditions.checkNotNull(flag);

Review comment:
       No need of null check here.

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMStorageConfig.java
##########
@@ -27,11 +28,7 @@
 import java.util.Properties;
 import java.util.UUID;
 
-import static org.apache.hadoop.ozone.OzoneConsts.PRIMARY_SCM_NODE_ID;
-import static org.apache.hadoop.ozone.OzoneConsts.SCM_CERT_SERIAL_ID;
-import static org.apache.hadoop.ozone.OzoneConsts.SCM_ID;
-import static org.apache.hadoop.ozone.OzoneConsts.STORAGE_DIR;
-
+import static org.apache.hadoop.ozone.OzoneConsts.*;

Review comment:
       Unnecessary, expand *

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -871,6 +874,7 @@ public static boolean scmBootstrap(OzoneConfiguration conf)
               getScmAddress(scmhaNodeDetails, conf), false);
         }
         scmStorageConfig.setPrimaryScmNodeId(scmInfo.getScmId());
+        scmStorageConfig.setSCMHAFlag(true);

Review comment:
       Do we need to set this flag only when HA enabled to avoid user mistakes of running bootStrap on non-HA cluster?

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/Storage.java
##########
@@ -250,6 +250,19 @@ public void initialize() throws IOException {
     storageInfo.writeTo(getVersionFile());
   }
 
+  /**
+   * Creates the Version file even if it exits,

Review comment:
       exits -> exists
   
   And remove "otherwise returns with IOException."




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

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