errose28 commented on code in PR #3499:
URL: https://github.com/apache/ozone/pull/3499#discussion_r900621640
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestStorageContainerManager.java:
##########
@@ -552,7 +560,11 @@ public void testSCMReinitializationWithHAUpgrade() throws
Exception {
Assert.assertEquals(clusterId.toString(), scmStore.getClusterID());
Assert.assertFalse(scmStore.isSCMHAEnabled());
+
conf.setBoolean(ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY, true);
+ // Non Ratis SCM -> Ratis SCM is not supported {@see HDDS-6695}
Review Comment:
This test should probably be reverted to its original state and commented
out with a note that it is not currently supported like the test below it. It's
called `testSCMReinitializationWithHAUpgrade` which is not a valid thing that
can be done right now.
In its current state it is just testing fresh installs with and without
Ratis enabled, which we already have tests for.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHANodeDetails.java:
##########
@@ -144,7 +149,48 @@ public static SCMHANodeDetails loadDefaultConfig(
return new SCMHANodeDetails(scmNodeDetails, Collections.emptyList());
}
- public static SCMHANodeDetails loadSCMHAConfig(OzoneConfiguration conf)
+ /** Validates SCM HA Config.
+ For Non Initialized SCM the value is taken directly based on the config
+ {@link org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY}
+ which defaults to
+ {@link org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_HA_ENABLE_DEFAULT}
+ For Previously Initialized SCM the values are taken from the version file
+ Ratis SCM -> Non Ratis SCM & vice versa is not supported
+ This values is validated with the config provided.
+ **/
+ private static void validateSCMHAConfig(SCMStorageConfig scmStorageConfig,
+ OzoneConfiguration conf) {
+ Storage.StorageState state = scmStorageConfig.getState();
+ boolean scmHAEnableDefault = state == Storage.StorageState.INITIALIZED
+ ? scmStorageConfig.isSCMHAEnabled()
+ : SCMHAUtils.isSCMHAEnabled(conf);
+ boolean scmHAEnabled = SCMHAUtils.isSCMHAEnabled(conf);
+
+ if (Storage.StorageState.INITIALIZED.equals(state) &&
+ scmHAEnabled != scmHAEnableDefault) {
+ String errorMessage = String.format("Current State of SCM: %s",
+ scmHAEnableDefault ? "SCM HA is enabled with Ratis"
+ : "SCM is running in Non HA without Ratis")
+ + " HA SCM -> Non HA SCM or " +
+ "Non HA SCM -> HA SCM is not supported";
Review Comment:
```suggestion
scmHAEnableDefault ? "SCM is enabled with Ratis"
: "SCM is running in Non HA without Ratis")
+ " Ratis SCM -> Non Ratis SCM or " +
"Non Ratis SCM -> Ratis SCM is not supported";
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java:
##########
@@ -135,6 +136,7 @@ public void shutdown() {
if (cluster != null) {
cluster.shutdown();
}
+ DefaultConfigManager.clearDefaultConfigs();
Review Comment:
Why is this change necessary?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHANodeDetails.java:
##########
@@ -144,7 +149,48 @@ public static SCMHANodeDetails loadDefaultConfig(
return new SCMHANodeDetails(scmNodeDetails, Collections.emptyList());
}
- public static SCMHANodeDetails loadSCMHAConfig(OzoneConfiguration conf)
+ /** Validates SCM HA Config.
+ For Non Initialized SCM the value is taken directly based on the config
+ {@link org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY}
+ which defaults to
+ {@link org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_HA_ENABLE_DEFAULT}
+ For Previously Initialized SCM the values are taken from the version file
+ Ratis SCM -> Non Ratis SCM & vice versa is not supported
+ This values is validated with the config provided.
+ **/
+ private static void validateSCMHAConfig(SCMStorageConfig scmStorageConfig,
+ OzoneConfiguration conf) {
+ Storage.StorageState state = scmStorageConfig.getState();
+ boolean scmHAEnableDefault = state == Storage.StorageState.INITIALIZED
+ ? scmStorageConfig.isSCMHAEnabled()
+ : SCMHAUtils.isSCMHAEnabled(conf);
+ boolean scmHAEnabled = SCMHAUtils.isSCMHAEnabled(conf);
+
+ if (Storage.StorageState.INITIALIZED.equals(state) &&
+ scmHAEnabled != scmHAEnableDefault) {
+ String errorMessage = String.format("Current State of SCM: %s",
+ scmHAEnableDefault ? "SCM HA is enabled with Ratis"
+ : "SCM is running in Non HA without Ratis")
+ + " HA SCM -> Non HA SCM or " +
+ "Non HA SCM -> HA SCM is not supported";
+ if (Strings.isNotEmpty(conf.get(ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY)))
{
+ throw new ConfigurationException(String.format("Invalid Config %s " +
+ "Provided ConfigValue: %s, Expected Config Value: %s. %s",
+ ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY, scmHAEnabled,
+ scmHAEnableDefault, errorMessage));
+ } else {
+ LOG.warn("Invalid Config {}, Expected Config Value: {}, Default Config
"
+ + "Value: {}. {}",
+ ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY,
+ scmHAEnableDefault, scmHAEnabled, errorMessage);
Review Comment:
```suggestion
LOG.warn("Invalid config {}. The config was not specified, but the
default value {} conflicts with the expected config value {}. Falling back to
the expected value. {}", ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY,
ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY_DEFAULT,
scmHAEnableDefault, errorMessage);
```
--
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]
