-
Notifications
You must be signed in to change notification settings - Fork 511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-11959. Remove tests for non-Ratis SCM #7612
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chungen0126 for the patch.
There are some leftover references in tests:
ozone/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestSCMSnapshot.java
Line 47 in 5b27f6d
conf.setBoolean(ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY, true); |
Line 69 in 5b27f6d
conf.setBoolean(ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY, true); |
Line 235 in 5b27f6d
conf.setBoolean(ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY, false); |
(please check if TestSecureOzoneCluster
works without this setting)
Also in MiniOzoneClusterImpl
:
ozone/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java
Lines 769 to 780 in 5b27f6d
//TODO: HDDS-6897 | |
//Disabling Ratis for only of MiniOzoneClusterImpl. | |
//MiniOzoneClusterImpl doesn't work with Ratis enabled SCM | |
if (StringUtils.isNotEmpty( | |
conf.get(ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY)) | |
&& SCMHAUtils.isSCMHAEnabled(conf)) { | |
scmStore.setSCMHAFlag(true); | |
scmStore.persistCurrentState(); | |
SCMRatisServerImpl.initialize(clusterId, scmId, | |
SCMHANodeDetails.loadSCMHAConfig(conf, scmStore) | |
.getLocalNodeDetails(), conf); | |
} |
Thanks @adoroszlai for reviewing.
I'm leaving the reference in MiniOzoneClusterImpl because that enabling SCM Ratis in all tests with
I'll convert this to draft and fix HDDS-6897 first. I think that would make the change clearer. |
5106805
to
ca68e85
Compare
There is only one |
I guess you mean when the config itself is removed. |
Yes, you're right, I wrote the wrong word. Thanks for the reminder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chungen0126 for the patch, mostly LGTM.
...e/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestStorageContainerManager.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java
Outdated
Show resolved
Hide resolved
* Test Recon SCM HA Snapshot Download implementation. | ||
*/ | ||
@Timeout(300) | ||
public class TestReconScmHASnapshot { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both TestReconScmHASnapshot and TestReconScmNonHASnapshot are covered by TestReconScmSnapshot.
Merged. Thanks @chungen0126 and @adoroszlai for review. |
What changes were proposed in this pull request?
Remove tests that exercise non-Ratis SCM.
Please describe your PR in detail:
OZONE_SCM_RATIS_PORT_KEY
andOZONE_SCM_GRPC_PORT_KEY
as a free port to avoid port conflict fromMiniOzoneClusterProvider
.TestSCMContainerManagerMetrics
,TestContainerStateMachine
, andTestReconTasks
)What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11959
How was this patch tested?
https://github.com/chungen0126/ozone/actions/runs/12621318024