You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/06/09 23:39:15 UTC
[GitHub] [ozone] swamirishi opened a new pull request, #3499: HDDS-6695: Enable SCM Ratis by default for new clusters only
swamirishi opened a new pull request, #3499:
URL: https://github.com/apache/ozone/pull/3499
## What changes were proposed in this pull request?
(Please fill in changes proposed in this fix)
## What is the link to the Apache JIRA
(Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HDDS-XXXX. Fix a typo in YYY.)
Please replace this section with the link to the Apache JIRA)
## How was this patch tested?
Unit Tests & Acceptance Test
(Please explain how this patch was tested. Ex: unit tests, manual tests)
(If this patch involves UI changes, please attach a screen-shot; otherwise, remove this)
--
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
[GitHub] [ozone] errose28 commented on pull request #3499: HDDS-6695. Enable SCM Ratis by default for new clusters only
Posted by GitBox <gi...@apache.org>.
errose28 commented on PR #3499:
URL: https://github.com/apache/ozone/pull/3499#issuecomment-1160885430
Thanks for working on this @swamirishi. LGTM pending green CI.
--
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
[GitHub] [ozone] errose28 commented on a diff in pull request #3499: HDDS-6695. Enable SCM Ratis by default for new clusters only
Posted by 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
[GitHub] [ozone] swamirishi commented on a diff in pull request #3499: HDDS-6695. Enable SCM Ratis by default for new clusters only
Posted by GitBox <gi...@apache.org>.
swamirishi commented on code in PR #3499:
URL: https://github.com/apache/ozone/pull/3499#discussion_r899415870
##########
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:
Have added a comment block where the configs are being validated.
--
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
[GitHub] [ozone] JyotinderSingh commented on a diff in pull request #3499: HDDS-6695. Enable SCM Ratis by default for new clusters only
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on code in PR #3499:
URL: https://github.com/apache/ozone/pull/3499#discussion_r894216055
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/conf/DefaultConfigManager.java:
##########
@@ -0,0 +1,21 @@
+package org.apache.hadoop.hdds.conf;
Review Comment:
Please include the apache license information in a block comment at the top of any new files you introduce.
```suggestion
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.hadoop.hdds.conf;
```
##########
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 " +
+ "Provided ConfigValue: %s, Expected Config Value: %s",
+ ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY, scmHAEnabled, scmHAEnableDefault));
+ } else if (Storage.StorageState.INITIALIZED == state
+ && scmHAEnabled != scmHAEnableDefault) {
+ LOG.warn("Invalid Config {}, Expected Config Value: {}, Default Config " +
+ "Value: {}", ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY,
+ scmHAEnableDefault, scmHAEnabled);
+ }
Review Comment:
I feel this if-else block can be simplified as follows for readability
```suggestion
String scmHAEnableConfig = conf.get(ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY);
if (state.equals(Storage.StorageState.INITIALIZED) &&
scmHAEnabled != scmHAEnableDefault)){
if (Strings.isNotEmpty(scmHAEnableConfig)) {
throw new ConfigurationException(String.format("Invalid Config %s " +
"Provided ConfigValue: %s, Expected Config Value: %s",
ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY, scmHAEnabled,
scmHAEnableDefault));
} else {
LOG.warn(
"Invalid Config {}, Expected Config Value: {}, Default Config " +
"Value: {}", ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY,
scmHAEnableDefault, scmHAEnabled);
}
}
```
--
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
[GitHub] [ozone] JyotinderSingh commented on pull request #3499: HDDS-6695: Enable SCM Ratis by default for new clusters only
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on PR #3499:
URL: https://github.com/apache/ozone/pull/3499#issuecomment-1152039904
Thank you for the contribution @swamirishi, please include a link to the related issue on ASF Jira and fill in the required details in the description of the PR.
Currently, the description holds the placeholder text that you are supposed to replace with details related to your changes.
This helps us track and understand the context for your changes.
--
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
[GitHub] [ozone] errose28 merged pull request #3499: HDDS-6695. Enable SCM Ratis by default for new clusters only
Posted by GitBox <gi...@apache.org>.
errose28 merged PR #3499:
URL: https://github.com/apache/ozone/pull/3499
--
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
[GitHub] [ozone] errose28 commented on a diff in pull request #3499: HDDS-6695. Enable SCM Ratis by default for new clusters only
Posted by GitBox <gi...@apache.org>.
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: 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