This is an automated email from the ASF dual-hosted git repository. sodonnell pushed a commit to branch HDDS-3816-ec in repository https://gitbox.apache.org/repos/asf/ozone.git
commit 3007b5c68dfda26218748bcc8ded7cd3115fe7c3 Author: Istvan Fajth <[email protected]> AuthorDate: Fri Dec 10 12:05:39 2021 +0100 HDDS-6081. EC: Fix ReplicationConfig related test failures that happened due to merging HDDS-5997 to the EC branch --- .../hadoop/hdds/client/ECReplicationConfig.java | 3 +- .../hadoop/hdds/client/ReplicationConfig.java | 9 +- .../hadoop/hdds/client/TestReplicationConfig.java | 184 +++++++++++++++++++-- .../hadoop/fs/ozone/TestRootedOzoneFileSystem.java | 4 +- 4 files changed, 179 insertions(+), 21 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ECReplicationConfig.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ECReplicationConfig.java index 93cef00..8690876 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ECReplicationConfig.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ECReplicationConfig.java @@ -92,7 +92,8 @@ public class ECReplicationConfig implements ReplicationConfig { final Matcher matcher = STRING_FORMAT.matcher(string); if (!matcher.matches()) { throw new IllegalArgumentException("EC replication config should be " + - "defined in the form rs-3-2-1024k, rs-6-3-1024; or rs-10-4-1024k"); + "defined in the form rs-3-2-1024k, rs-6-3-1024; or rs-10-4-1024k." + + "Provided configuration was: " + string); } try { diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ReplicationConfig.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ReplicationConfig.java index 8e368fc..c031ed6 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ReplicationConfig.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ReplicationConfig.java @@ -123,9 +123,12 @@ public interface ReplicationConfig { */ static ReplicationConfig adjustReplication( ReplicationConfig config, short replication, ConfigurationSource conf) { - return parse( - ReplicationType.valueOf(config.getReplicationType().toString()), - Short.toString(replication), conf); + ReplicationType replicationType = + ReplicationType.valueOf(config.getReplicationType().toString()); + if (replicationType.equals(ReplicationType.EC)) { + return config; + } + return parse(replicationType, Short.toString(replication), conf); } /** diff --git a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/client/TestReplicationConfig.java b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/client/TestReplicationConfig.java index 6e6f7b5..19cc13e 100644 --- a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/client/TestReplicationConfig.java +++ b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/client/TestReplicationConfig.java @@ -17,10 +17,13 @@ */ package org.apache.hadoop.hdds.client; +import org.apache.hadoop.hdds.client.ECReplicationConfig.EcCodec; import org.apache.hadoop.hdds.conf.ConfigurationSource; import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos; import org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor; import org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationType; +import org.junit.Assume; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -36,8 +39,11 @@ import static org.junit.Assert.assertThrows; @RunWith(Parameterized.class) public class TestReplicationConfig { + private static final int MB = 1024*1024; + private static final int KB = 1024; + @SuppressWarnings("checkstyle:VisibilityModifier") - @Parameterized.Parameter() + @Parameterized.Parameter public String type; @SuppressWarnings("checkstyle:VisibilityModifier") @@ -46,20 +52,46 @@ public class TestReplicationConfig { @SuppressWarnings("checkstyle:VisibilityModifier") @Parameterized.Parameter(2) + public String codec; + + @SuppressWarnings("checkstyle:VisibilityModifier") + @Parameterized.Parameter(3) + public int data; + + @SuppressWarnings("checkstyle:VisibilityModifier") + @Parameterized.Parameter(4) + public int parity; + + @SuppressWarnings("checkstyle:VisibilityModifier") + @Parameterized.Parameter(5) + public int chunkSize; + + @SuppressWarnings("checkstyle:VisibilityModifier") + @Parameterized.Parameter(6) public Class<?> replicationConfigClass; + //NOTE: if a new cunckSize is used/added in the parameters other than KB or MB + // please revisit the method createECDescriptor, to handle the new chunkSize. @Parameterized.Parameters(name = "{0}/{1}") public static Object[][] parameters() { return new Object[][] { - {"RATIS", "ONE", RatisReplicationConfig.class }, - {"RATIS", "THREE", RatisReplicationConfig.class}, - {"STAND_ALONE", "ONE", StandaloneReplicationConfig.class}, - {"STAND_ALONE", "THREE", StandaloneReplicationConfig.class} + {"RATIS", "ONE", null, 0, 0, 0, RatisReplicationConfig.class }, + {"RATIS", "THREE", null, 0, 0, 0, RatisReplicationConfig.class}, + {"STAND_ALONE", "ONE", null, 0, 0, 0, + StandaloneReplicationConfig.class}, + {"STAND_ALONE", "THREE", null, 0, 0, 0, + StandaloneReplicationConfig.class}, + {"EC", "RS-3-2-1024K", "RS", 3, 2, MB, ECReplicationConfig.class}, + {"EC", "RS-3-2-1024", "RS", 3, 2, KB, ECReplicationConfig.class}, + {"EC", "RS-6-3-1024K", "RS", 6, 3, MB, ECReplicationConfig.class}, + {"EC", "RS-6-3-1024", "RS", 6, 3, KB, ECReplicationConfig.class}, + {"EC", "RS-10-4-1024K", "RS", 10, 4, MB, ECReplicationConfig.class}, + {"EC", "RS-10-4-1024", "RS", 10, 4, KB, ECReplicationConfig.class}, }; } @Test - public void testGetDefaultShouldReturnNullIfNotSetClientSide() { + public void testGetDefaultShouldReturnRatisThreeIfNotSetClientSide() { OzoneConfiguration conf = new OzoneConfiguration(); ReplicationConfig replicationConfig = ReplicationConfig.getDefault(conf); @@ -70,7 +102,8 @@ public class TestReplicationConfig { } @Test - public void testGetDefaultShouldCreateReplicationConfFromCustomConfValues() { + public void testGetDefaultShouldCreateReplicationConfFromConfValues() { + assumeRatisOrStandaloneType(); OzoneConfiguration conf = new OzoneConfiguration(); conf.set(OZONE_REPLICATION_TYPE, type); conf.set(OZONE_REPLICATION, factor); @@ -83,7 +116,22 @@ public class TestReplicationConfig { } @Test + public void testGetDefaultShouldCreateECReplicationConfFromConfValues() { + assumeECType(); + + OzoneConfiguration conf = new OzoneConfiguration(); + conf.set(OZONE_REPLICATION_TYPE, type); + conf.set(OZONE_REPLICATION, ecDescriptor()); + + ReplicationConfig replicationConfig = ReplicationConfig.getDefault(conf); + + validate(replicationConfig, + EcCodec.valueOf(codec), data, parity, chunkSize); + } + + @Test public void deserialize() { + assumeRatisOrStandaloneType(); final ReplicationConfig replicationConfig = ReplicationConfig.fromProtoTypeAndFactor( ReplicationType.valueOf(type), @@ -95,7 +143,25 @@ public class TestReplicationConfig { } @Test + public void deserializeEC() { + assumeECType(); + HddsProtos.ECReplicationConfig proto = + HddsProtos.ECReplicationConfig.newBuilder() + .setCodec(codec) + .setData(data) + .setParity(parity) + .setEcChunkSize(chunkSize) + .build(); + + ReplicationConfig config = ReplicationConfig + .fromProto(ReplicationType.EC, null, proto); + + validate(config, EcCodec.valueOf(codec), data, parity, chunkSize); + } + + @Test public void fromJavaObjects() { + assumeRatisOrStandaloneType(); final ReplicationConfig replicationConfig = ReplicationConfig.fromTypeAndFactor( org.apache.hadoop.hdds.client.ReplicationType.valueOf(type), @@ -108,6 +174,7 @@ public class TestReplicationConfig { @Test public void testParseFromTypeAndFactorAsString() { + assumeRatisOrStandaloneType(); ConfigurationSource conf = new OzoneConfiguration(); ReplicationConfig replicationConfig = ReplicationConfig.parse( org.apache.hadoop.hdds.client.ReplicationType.valueOf(type), @@ -120,10 +187,11 @@ public class TestReplicationConfig { @Test public void testParseFromTypeAndFactorAsStringifiedInteger() { + assumeRatisOrStandaloneType(); ConfigurationSource conf = new OzoneConfiguration(); String f = - factor == "ONE" ? "1" - : factor == "THREE" ? "3" + factor.equals("ONE") ? "1" + : factor.equals("THREE") ? "3" : "Test adjustment needed!"; ReplicationConfig replicationConfig = ReplicationConfig.parse( @@ -136,7 +204,27 @@ public class TestReplicationConfig { } @Test + public void testParseECReplicationConfigFromString() { + assumeECType(); + + ConfigurationSource conf = new OzoneConfiguration(); + ReplicationConfig repConfig = ReplicationConfig.parse( + org.apache.hadoop.hdds.client.ReplicationType.EC, ecDescriptor(), conf); + + validate(repConfig, EcCodec.valueOf(codec), data, parity, chunkSize); + } + + /** + * The adjustReplication is a method that is used by RootedOzoneFileSystem + * to adjust the bucket's default replication config if needed. + * + * As we define, if the bucket's default replication configuration is RATIS + * or STAND_ALONE, then replica count can be adjusted with the replication + * factor. + */ + @Test public void testAdjustReplication() { + assumeRatisOrStandaloneType(); ConfigurationSource conf = new OzoneConfiguration(); ReplicationConfig replicationConfig = ReplicationConfig.parse( org.apache.hadoop.hdds.client.ReplicationType.valueOf(type), @@ -154,6 +242,30 @@ public class TestReplicationConfig { } /** + * The adjustReplication is a method that is used by RootedOzoneFileSystem + * to adjust the bucket's default replication config if needed. + * + * As we define, if the bucket's default replication configuration is EC, + * then the client can not adjust the configuration via the replication + * factor. + */ + @Test + public void testAdjustECReplication() { + assumeECType(); + ConfigurationSource conf = new OzoneConfiguration(); + ReplicationConfig replicationConfig = ReplicationConfig.parse( + org.apache.hadoop.hdds.client.ReplicationType.EC, ecDescriptor(), conf); + + validate( + ReplicationConfig.adjustReplication(replicationConfig, (short) 3, conf), + EcCodec.valueOf(codec), data, parity, chunkSize); + + validate( + ReplicationConfig.adjustReplication(replicationConfig, (short) 1, conf), + EcCodec.valueOf(codec), data, parity, chunkSize); + } + + /** * This is a bit of a tricky test in the parametrized environment. * The goal is to ensure that the following methods do validation while * creating the ReplicationConfig: getDefault, adjustReplication, parse. @@ -165,17 +277,21 @@ public class TestReplicationConfig { */ @Test public void testValidationBasedOnConfig() { + assumeRatisOrStandaloneType(); OzoneConfiguration conf = new OzoneConfiguration(); conf.set(OZONE_REPLICATION+".allowed-configs", "^STANDALONE/ONE|RATIS/THREE$"); conf.set(OZONE_REPLICATION, factor); conf.set(OZONE_REPLICATION_TYPE, type); + // in case of allowed configurations if ((type.equals("RATIS") && factor.equals("THREE")) || (type.equals("STAND_ALONE") && factor.equals("ONE"))) { ReplicationConfig replicationConfig = ReplicationConfig.parse( org.apache.hadoop.hdds.client.ReplicationType.valueOf(type), factor, conf); + // check if adjust throws exception when adjusting to a config that is + // not allowed if (type.equals("RATIS")) { assertThrows(IllegalArgumentException.class, () -> ReplicationConfig @@ -185,27 +301,33 @@ public class TestReplicationConfig { () -> ReplicationConfig .adjustReplication(replicationConfig, (short) 3, conf)); } - ReplicationConfig.fromTypeAndFactor( - org.apache.hadoop.hdds.client.ReplicationType.valueOf(type), - org.apache.hadoop.hdds.client.ReplicationFactor.valueOf(factor)); - ReplicationConfig.fromProtoTypeAndFactor( - ReplicationType.valueOf(type), ReplicationFactor.valueOf(factor)); ReplicationConfig.getDefault(conf); } else { + // parse should fail in case of a configuration that is not allowed. assertThrows(IllegalArgumentException.class, () -> ReplicationConfig.parse( org.apache.hadoop.hdds.client.ReplicationType.valueOf(type), factor, conf)); + // default can not be a configuration that is not allowed. assertThrows(IllegalArgumentException.class, () -> ReplicationConfig.getDefault(conf)); } + + // From proto and java objects, we need to be able to create replication + // configs even though they are not allowed, as there might have been + // keys, that were created earlier when the config was allowed. + ReplicationConfig.fromTypeAndFactor( + org.apache.hadoop.hdds.client.ReplicationType.valueOf(type), + org.apache.hadoop.hdds.client.ReplicationFactor.valueOf(factor)); + ReplicationConfig.fromProtoTypeAndFactor( + ReplicationType.valueOf(type), ReplicationFactor.valueOf(factor)); + // CHAINED replication type is not supported by ReplicationConfig. assertThrows(RuntimeException.class, () -> ReplicationConfig.parse( org.apache.hadoop.hdds.client.ReplicationType.CHAINED, "", conf)); } - private void validate(ReplicationConfig replicationConfig, org.apache.hadoop.hdds.client.ReplicationType expectedType, org.apache.hadoop.hdds.client.ReplicationFactor expectedFactor) { @@ -231,4 +353,36 @@ public class TestReplicationConfig { ((ReplicatedReplicationConfig) replicationConfig) .getReplicationFactor().name()); } + + private void validate(ReplicationConfig replicationConfig, + EcCodec expectedCodec, + int expectedData, int expectedParity, int expectedChunkSize) { + + assertEquals(ECReplicationConfig.class, replicationConfig.getClass()); + assertEquals(ReplicationType.EC, replicationConfig.getReplicationType()); + + ECReplicationConfig ecReplicationConfig = + (ECReplicationConfig) replicationConfig; + + assertEquals(expectedCodec, ecReplicationConfig.getCodec()); + assertEquals(expectedData, ecReplicationConfig.getData()); + assertEquals(expectedParity, ecReplicationConfig.getParity()); + assertEquals(expectedChunkSize, ecReplicationConfig.getEcChunkSize()); + + assertEquals(expectedData + expectedParity, + replicationConfig.getRequiredNodes()); + } + + private String ecDescriptor() { + return codec.toUpperCase() + "-" + data + "-" + parity + "-" + + (chunkSize == MB ? "1024K" : "1024"); + } + + private void assumeRatisOrStandaloneType() { + Assume.assumeTrue(type.equals("RATIS") || type.equals("STAND_ALONE")); + } + + private void assumeECType() { + Assume.assumeTrue(type.equals("EC")); + } } diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java index 4ca2193..d720b7b 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java @@ -1639,8 +1639,8 @@ public class TestRootedOzoneFileSystem { OFSPath ofsPath = new OFSPath(vol + "/" + buck + "/test"); final OzoneBucket bucket = adapter.getBucket(ofsPath, false); final OzoneKeyDetails key = bucket.getKey(ofsPath.getKeyName()); - Assert.assertEquals(key.getReplicationConfig().getReplicationType().name(), - ReplicationType.EC.name()); + Assert.assertEquals(ReplicationType.EC.name(), + key.getReplicationConfig().getReplicationType().name()); } public void testNonPrivilegedUserMkdirCreateBucket() throws IOException { --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
