You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "chungen0126 (via GitHub)" <gi...@apache.org> on 2024/12/23 21:16:08 UTC
[PR] HDDS-11959. Remove tests for non-Ratis SCM [ozone]
chungen0126 opened a new pull request, #7612:
URL: https://github.com/apache/ozone/pull/7612
## What changes were proposed in this pull request?
Remove tests that exercise non-Ratis SCM.
Please describe your PR in detail:
1. Remove tests for non-Ratis SCM
2. Remove unnecessary "Enable SCM Ratis" config.
3. Integration tests with SCM HA Enabled not working with `MiniOzoneClusterImpl`. (should be fixed in [HDDS-6897](https://issues.apache.org/jira/browse/HDDS-6897))
4. Disable test cases in SCM unit tests where test implementation is specific to non-Ratis mode (should be updated in [HDDS-11986](https://issues.apache.org/jira/browse/HDDS-11986))
## 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/12473049289
--
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
Re: [PR] HDDS-11959. Remove tests for non-Ratis SCM [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on PR #7612:
URL: https://github.com/apache/ozone/pull/7612#issuecomment-2574956230
> > It should be removed after the `TestOzoneConfigurationFields` is removed.
>
> I guess you mean when the config itself is removed.
Yes, you're right, I wrote the wrong word. Thanks for the reminder.
--
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
Re: [PR] HDDS-11959. Remove tests for non-Ratis SCM [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #7612:
URL: https://github.com/apache/ozone/pull/7612#discussion_r1907927302
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconScmHASnapshot.java:
##########
@@ -1,65 +0,0 @@
-/**
- * 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
- * <p>
- * http://www.apache.org/licenses/LICENSE-2.0
- * <p>
- * 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.ozone.recon;
-
-import org.apache.hadoop.hdds.conf.OzoneConfiguration;
-import org.apache.hadoop.hdds.scm.ScmConfigKeys;
-import org.apache.hadoop.ozone.MiniOzoneCluster;
-
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.AfterEach;
-import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Timeout;
-
-import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY;
-
-/**
- * Test Recon SCM HA Snapshot Download implementation.
- */
-@Timeout(300)
-public class TestReconScmHASnapshot {
Review Comment:
Both TestReconScmHASnapshot and TestReconScmNonHASnapshot are covered by TestReconScmSnapshot.
--
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
Re: [PR] HDDS-11959. Remove tests for non-Ratis SCM [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7612:
URL: https://github.com/apache/ozone/pull/7612#discussion_r1905991580
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestStorageContainerManager.java:
##########
@@ -666,15 +662,21 @@ public void testScmInfo(@TempDir Path tempDir) throws Exception {
SCMHANodeDetails.loadSCMHAConfig(conf, scmStore)
.getLocalNodeDetails(), conf);
StorageContainerManager scm = HddsTestUtils.getScmSimple(conf);
- scm.start();
- //Reads the SCM Info from SCM instance
- ScmInfo scmInfo = scm.getClientProtocolServer().getScmInfo();
- assertEquals(clusterId, scmInfo.getClusterId());
- assertEquals(scmId, scmInfo.getScmId());
-
- String expectedVersion = HddsVersionInfo.HDDS_VERSION_INFO.getVersion();
- String actualVersion = scm.getSoftwareVersion();
- assertEquals(expectedVersion, actualVersion);
+ try {
+ scm.start();
+ //Reads the SCM Info from SCM instance
+ ScmInfo scmInfo = scm.getClientProtocolServer().getScmInfo();
+ assertEquals(clusterId, scmInfo.getClusterId());
+ assertEquals(scmId, scmInfo.getScmId());
+
+ String expectedVersion = HddsVersionInfo.HDDS_VERSION_INFO.getVersion();
+ String actualVersion = scm.getSoftwareVersion();
+ assertEquals(expectedVersion, actualVersion);
+ } finally {
+ if (scmStore != null) {
+ scm.stop();
Review Comment:
How can `scmStore == null` here?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java:
##########
@@ -755,15 +751,13 @@ protected void initializeScmStorage(SCMStorageConfig scmStore)
//TODO: HDDS-6897
//Disabling Ratis for only of MiniOzoneClusterImpl.
//MiniOzoneClusterImpl doesn't work with Ratis enabled SCM
Review Comment:
Comment seems to be leftover.
--
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
Re: [PR] HDDS-11959. Remove tests for non-Ratis SCM [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on PR #7612:
URL: https://github.com/apache/ozone/pull/7612#issuecomment-2560909757
Thanks @adoroszlai for reviewing.
> There are some leftover references in tests:
>
> https://github.com/apache/ozone/blob/5b27f6d938c67028f105767f104f65efb9f5637d/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestSCMSnapshot.java#L47
>
> https://github.com/apache/ozone/blob/5b27f6d938c67028f105767f104f65efb9f5637d/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestSCMInstallSnapshot.java#L69
>
> (please check if `TestSecureOzoneCluster` works without this setting)
>
> Also in `MiniOzoneClusterImpl`:
>
> https://github.com/apache/ozone/blob/5b27f6d938c67028f105767f104f65efb9f5637d/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java#L769-L780
I'm leaving the reference in MiniOzoneClusterImpl because that enabling SCM Ratis in all tests with `MiniOzoneClusterImpl` leads to many tests failures. [TestSCMSnapshot.java](https://github.com/apache/ozone/blob/5b27f6d938c67028f105767f104f65efb9f5637d/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestSCMSnapshot.java#L47) and [TestSCMInstallSnapshot.java](https://github.com/apache/ozone/blob/5b27f6d938c67028f105767f104f65efb9f5637d/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestSCMInstallSnapshot.java#L69) should set `OZONE_SCM_HA_ENABLE_KEY ` to true because `MiniOzoneClusterImpl` is non-Ratis SCM by default.
I believe we should fix this in [HDDS-6897](https://issues.apache.org/jira/browse/HDDS-6897).
> (please check if TestSecureOzoneCluster works without this setting)
`TestSecureOzoneCluster` doesn't work with `OZONE_SCM_HA_ENABLE_KEY` set to true but works without it. However, `MiniOzoneClusterImpl` is non-Ratis SCM by default.
I'll convert this to draft and fix [HDDS-6897](https://issues.apache.org/jira/browse/HDDS-6897) first. I think that would make the change clearer.
--
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
Re: [PR] HDDS-11959. Remove tests for non-Ratis SCM [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #7612:
URL: https://github.com/apache/ozone/pull/7612#issuecomment-2578932922
Merged. Thanks @chungen0126 and @adoroszlai for review.
--
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
Re: [PR] HDDS-11959. Remove tests for non-Ratis SCM [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #7612:
URL: https://github.com/apache/ozone/pull/7612#issuecomment-2574934537
> It should be removed after the `TestOzoneConfigurationFields` is removed.
I guess you mean when the config itself is removed.
--
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
Re: [PR] HDDS-11959. Remove tests for non-Ratis SCM [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on PR #7612:
URL: https://github.com/apache/ozone/pull/7612#issuecomment-2574926203
There is only one `OZONE_SCM_HA_ENABLE_KEY` in all the tests which is in `TestOzoneConfigurationFields`. It should be removed after the `TestOzoneConfigurationFields` is removed.
--
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
Re: [PR] HDDS-11959. Remove tests for non-Ratis SCM [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #7612:
URL: https://github.com/apache/ozone/pull/7612#discussion_r1906828480
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java:
##########
@@ -755,15 +751,13 @@ protected void initializeScmStorage(SCMStorageConfig scmStore)
//TODO: HDDS-6897
//Disabling Ratis for only of MiniOzoneClusterImpl.
//MiniOzoneClusterImpl doesn't work with Ratis enabled SCM
Review Comment:
Removed.
--
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
Re: [PR] HDDS-11959. Remove tests for non-Ratis SCM [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #7612:
URL: https://github.com/apache/ozone/pull/7612#discussion_r1906829159
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestStorageContainerManager.java:
##########
@@ -666,15 +662,21 @@ public void testScmInfo(@TempDir Path tempDir) throws Exception {
SCMHANodeDetails.loadSCMHAConfig(conf, scmStore)
.getLocalNodeDetails(), conf);
StorageContainerManager scm = HddsTestUtils.getScmSimple(conf);
- scm.start();
- //Reads the SCM Info from SCM instance
- ScmInfo scmInfo = scm.getClientProtocolServer().getScmInfo();
- assertEquals(clusterId, scmInfo.getClusterId());
- assertEquals(scmId, scmInfo.getScmId());
-
- String expectedVersion = HddsVersionInfo.HDDS_VERSION_INFO.getVersion();
- String actualVersion = scm.getSoftwareVersion();
- assertEquals(expectedVersion, actualVersion);
+ try {
+ scm.start();
+ //Reads the SCM Info from SCM instance
+ ScmInfo scmInfo = scm.getClientProtocolServer().getScmInfo();
+ assertEquals(clusterId, scmInfo.getClusterId());
+ assertEquals(scmId, scmInfo.getScmId());
+
+ String expectedVersion = HddsVersionInfo.HDDS_VERSION_INFO.getVersion();
+ String actualVersion = scm.getSoftwareVersion();
+ assertEquals(expectedVersion, actualVersion);
+ } finally {
+ if (scmStore != null) {
+ scm.stop();
Review Comment:
Removed.
--
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
Re: [PR] HDDS-11959. Remove tests for non-Ratis SCM [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang merged PR #7612:
URL: https://github.com/apache/ozone/pull/7612
--
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