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