adoroszlai commented on code in PR #7831:
URL: https://github.com/apache/ozone/pull/7831#discussion_r1947099944
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java:
##########
@@ -383,6 +383,15 @@ private StorageContainerManager(OzoneConfiguration conf,
"failure.", ResultCodes.SCM_NOT_INITIALIZED);
}
+ // Initialize Ratis if needed.
+ // This is for the clusters which got upgraded from older version of Ozone.
+ // We enable Ratis by default.
+ if (!scmStorageConfig.isSCMHAEnabled()) {
+ initializeRatis(conf);
+ // Since we have initialized Ratis, we have to reload StorageConfig
+ scmStorageConfig = new SCMStorageConfig(conf);
Review Comment:
Would it make sense for `initializeRatis` to return the `SCMStorageConfig`
it creates, and use that instance in the assignment?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java:
##########
@@ -1312,26 +1310,19 @@ public static boolean scmInit(OzoneConfiguration conf,
return false;
}
} else {
- clusterId = scmStorageConfig.getClusterID();
- final boolean isSCMHAEnabled = scmStorageConfig.isSCMHAEnabled();
// Initialize security if security is enabled later.
initializeSecurityIfNeeded(conf, scmStorageConfig, selfHostName, true);
- if (SCMHAUtils.isSCMHAEnabled(conf) && !isSCMHAEnabled) {
- SCMRatisServerImpl.initialize(scmStorageConfig.getClusterID(),
- scmStorageConfig.getScmId(), haDetails.getLocalNodeDetails(),
- conf);
- scmStorageConfig.setSCMHAFlag(true);
- scmStorageConfig.setPrimaryScmNodeId(scmStorageConfig.getScmId());
- scmStorageConfig.forceInitialize();
+ // Enable Ratis if it's not already enabled.
+ if (!scmStorageConfig.isSCMHAEnabled()) {
+ initializeRatis(conf);
Review Comment:
Again, here I think `scmStorageConfig` should be reassigned.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java:
##########
@@ -1291,16 +1298,7 @@ public static boolean scmInit(OzoneConfiguration conf,
scmStorageConfig.setPrimaryScmNodeId(scmStorageConfig.getScmId());
scmStorageConfig.initialize();
-
- if (SCMHAUtils.isSCMHAEnabled(conf)) {
- SCMRatisServerImpl.initialize(scmStorageConfig.getClusterID(),
- scmStorageConfig.getScmId(), haDetails.getLocalNodeDetails(),
- conf);
- scmStorageConfig = new SCMStorageConfig(conf);
- scmStorageConfig.setSCMHAFlag(true);
- // Do force initialize to persist SCM_HA flag.
- scmStorageConfig.forceInitialize();
- }
+ initializeRatis(conf);
Review Comment:
Shouldn't `scmStorageConfig` be reassigned here? (Maybe it's not important,
since we just log a few items and exit, but I think it would be future-proof.)
--
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]
