[GitHub] [ozone] errose28 commented on a diff in pull request #3499: HDDS-6695. Enable SCM Ratis by default for new clusters only

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

errose28 commented on code in PR #3499:
URL: https://github.com/apache/ozone/pull/3499#discussion_r894978177


##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHANodeDetails.java:
##########
@@ -144,7 +151,8 @@ public static SCMHANodeDetails loadDefaultConfig(
     return new SCMHANodeDetails(scmNodeDetails, Collections.emptyList());
   }
 
-  public static SCMHANodeDetails loadSCMHAConfig(OzoneConfiguration conf)
+  public static SCMHANodeDetails loadSCMHAConfig(
+      OzoneConfiguration conf, Optional<SCMStorageConfig> storageConfig)

Review Comment:
   Looks like the Optional is only empty for tests right? Can we make `storageConfig` a required parameter and have the tests pass `new SCMStorageConfig(conf)`? This makes it clear to the production code that they must specify a storage config for correct behavior.



##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAUtils.java:
##########
@@ -89,7 +90,8 @@ private SCMHAUtils() {
   // Check if SCM HA is enabled.
   public static boolean isSCMHAEnabled(ConfigurationSource conf) {
     return conf.getBoolean(ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY,
-        ScmConfigKeys.OZONE_SCM_HA_ENABLE_DEFAULT);
+        DefaultConfigManager.getValue(ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY,

Review Comment:
   This might be a good place to add a block comment explaining in which cases Ratis will or will not be used by default based on user configuration and cluster state.



##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java:
##########
@@ -454,7 +456,7 @@ public final class ScmConfigKeys {
   public static final String OZONE_SCM_HA_ENABLE_KEY
       = "ozone.scm.ratis.enable";
   public static final boolean OZONE_SCM_HA_ENABLE_DEFAULT
-      = false;
+      = true;

Review Comment:
   Let's put a comment here saying that we will override this default to false if the SCM is not new or already using Ratis.



##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHANodeDetails.java:
##########
@@ -158,7 +166,28 @@ public static SCMHANodeDetails loadSCMHAConfig(OzoneConfiguration conf)
         ScmConfigKeys.OZONE_SCM_DEFAULT_SERVICE_ID);
 
     LOG.info("ServiceID for StorageContainerManager is {}", localScmServiceId);
-
+    if(!storageConfig.isPresent()){
+      storageConfig = Optional.of(new SCMStorageConfig(conf));
+    }
+    Storage.StorageState state = storageConfig.get().getState();
+    boolean scmHAEnableDefault = state == Storage.StorageState.INITIALIZED
+        ? storageConfig.get().isSCMHAEnabled()
+        : SCMHAUtils.isSCMHAEnabled(conf);
+    boolean scmHAEnabled = SCMHAUtils.isSCMHAEnabled(conf);
+    if (Storage.StorageState.INITIALIZED == state
+        && Strings.isNotEmpty(conf.get(ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY))
+        && scmHAEnabled != scmHAEnableDefault) {
+      throw new ConfigurationException(String.format("Invalid Config %s " +

Review Comment:
   I think we should make these messages more explicit. For the exception, let's tell the user why the config is invalid (they were previously using Ratis and now they are trying not to). For the warning, we should indicate that the config was not specified, so we are using this value based on the previous cluster state.



##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMHAConfiguration.java:
##########
@@ -259,4 +272,59 @@ public void testHAWithSamePortConfig() throws Exception {
 
 
   }
+
+  @Test
+  public void testRatisEnabledDefaultConfigWithoutInitializedSCM() throws IOException, NoSuchFieldException, IllegalAccessException {
+    SCMStorageConfig scmStorageConfig = Mockito.mock(SCMStorageConfig.class);
+    Mockito.when(scmStorageConfig.getState()).thenReturn(Storage.StorageState.NOT_INITIALIZED);
+    SCMHANodeDetails.loadSCMHAConfig(conf, Optional.of(scmStorageConfig));
+    Assert.assertEquals(SCMHAUtils.isSCMHAEnabled(conf), ScmConfigKeys.OZONE_SCM_HA_ENABLE_DEFAULT);
+    HddsTestUtils.setField(DefaultConfigManager.class, "configDefaultMap",

Review Comment:
   Instead of using reflection to clear the test state, can we just put each section that needs a clean slate in its own test? Another option could be an `@VisibleForTesting clearDefaults` method in the `DefaultConfigManager`.



-- 
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: issues-unsubscribe@ozone.apache.org

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