adoroszlai commented on code in PR #8559:
URL: https://github.com/apache/ozone/pull/8559#discussion_r2144298521
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerOperationClient.java:
##########
@@ -221,6 +221,35 @@ public ContainerWithPipeline
createContainer(HddsProtos.ReplicationType type,
}
}
+ @Override
+ public ContainerWithPipeline createContainer(ReplicationConfig
replicationConfig, String owner) throws IOException {
+ XceiverClientSpi client = null;
+ XceiverClientManager clientManager = getXceiverClientManager();
+ try {
+ ContainerWithPipeline containerWithPipeline;
+ if (replicationConfig.getReplicationType() ==
HddsProtos.ReplicationType.EC) {
+ containerWithPipeline =
+ storageContainerLocationClient.allocateContainer(replicationConfig,
+ owner);
+ } else {
+ containerWithPipeline =
+
storageContainerLocationClient.allocateContainer(replicationConfig.getReplicationType(),
+
HddsProtos.ReplicationFactor.valueOf(replicationConfig.getReplication()),
+ owner);
+ }
Review Comment:
`if` is not needed, call the new `allocateContainer(ReplicationConfig,
String)` method in both cases.
Also, to clarify my [earlier
comment](https://github.com/apache/ozone/pull/8559#discussion_r2125815386):
please change the existing `createContainer` method to call the new one with
`ReplicationConfig`.
https://github.com/apache/ozone/blob/a5444cf1f6d9fd7cefb33774f06dbe2467529fb6/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerOperationClient.java#L201-L222
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestContainerOperations.java:
##########
@@ -265,4 +267,25 @@ public void testNodeOperationalStates() throws Exception {
nm.setNodeOperationalState(node, originalState);
}
}
+
+ @Test
+ public void testCreateRatis() throws Exception {
+ ContainerWithPipeline container = storageClient.createContainer(
+
ReplicationConfig.fromProtoTypeAndFactor(HddsProtos.ReplicationType.RATIS,
HddsProtos.ReplicationFactor.THREE),
+ OzoneConsts.OZONE);
+
+ assertEquals(container.getContainerInfo().getContainerID(),
+
storageClient.getContainer(container.getContainerInfo().getContainerID())
+ .getContainerID());
+ }
+
+ @Test
+ public void testCreateEC() throws Exception {
+ ECReplicationConfig ecConfig = new ECReplicationConfig("RS-3-2-1024k");
Review Comment:
Please use `new ECReplicationConfig(3, 2)`. (Also in
`TestAllocateContainer`.)
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestAllocateContainer.java:
##########
@@ -73,4 +75,25 @@ public void testAllocateNull() {
SCMTestUtils.getReplicationType(conf),
SCMTestUtils.getReplicationFactor(conf), null));
}
+
+ @Test
+ public void testAllocateRatis() throws Exception {
+ ContainerWithPipeline container =
+
storageContainerLocationClient.allocateContainer(HddsProtos.ReplicationType.RATIS,
+ HddsProtos.ReplicationFactor.THREE, OzoneConsts.OZONE);
+
+ assertNotNull(container);
+ assertNotNull(container.getPipeline().getFirstNode());
+ }
+
+ @Test
+ public void testAllocateEC() throws Exception {
+ ECReplicationConfig ecReplicationConfig = new
ECReplicationConfig("RS-3-2-1024k");
+
+ ContainerWithPipeline container =
+ storageContainerLocationClient.allocateContainer(ecReplicationConfig,
OzoneConsts.OZONE);
+
+ assertNotNull(container);
+ assertNotNull(container.getPipeline().getFirstNode());
+ }
Review Comment:
Please reduce duplication.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocolServerSideTranslatorPB.java:
##########
@@ -753,9 +753,15 @@ public GetContainerReplicasResponseProto
getContainerReplicas(
public ContainerResponseProto allocateContainer(ContainerRequestProto
request,
int clientVersion) throws IOException {
- ContainerWithPipeline cp = impl
- .allocateContainer(request.getReplicationType(),
- request.getReplicationFactor(), request.getOwner());
+ ContainerWithPipeline cp;
+ if (request.getReplicationType() == HddsProtos.ReplicationType.EC) {
+ cp = impl
+ .allocateContainer(new
ECReplicationConfig(request.getEcReplicationConfig()), request.getOwner());
+ } else {
+ cp = impl
+ .allocateContainer(request.getReplicationType(),
+ request.getReplicationFactor(), request.getOwner());
+ }
Review Comment:
No need for `if-else`, use `ReplicationConfig.fromProto` and call the new
`allocateContainer` method.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestContainerOperations.java:
##########
@@ -265,4 +267,25 @@ public void testNodeOperationalStates() throws Exception {
nm.setNodeOperationalState(node, originalState);
}
}
+
+ @Test
+ public void testCreateRatis() throws Exception {
+ ContainerWithPipeline container = storageClient.createContainer(
+
ReplicationConfig.fromProtoTypeAndFactor(HddsProtos.ReplicationType.RATIS,
HddsProtos.ReplicationFactor.THREE),
+ OzoneConsts.OZONE);
+
+ assertEquals(container.getContainerInfo().getContainerID(),
+
storageClient.getContainer(container.getContainerInfo().getContainerID())
+ .getContainerID());
+ }
+
+ @Test
+ public void testCreateEC() throws Exception {
+ ECReplicationConfig ecConfig = new ECReplicationConfig("RS-3-2-1024k");
+ ContainerWithPipeline container = storageClient.createContainer(ecConfig,
OzoneConsts.OZONE);
+
+ assertEquals(container.getContainerInfo().getContainerID(),
+
storageClient.getContainer(container.getContainerInfo().getContainerID())
+ .getContainerID());
+ }
Review Comment:
Please reduce duplication.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java:
##########
@@ -222,13 +223,33 @@ private ScmContainerLocationResponse submitRpcRequest(
public ContainerWithPipeline allocateContainer(
HddsProtos.ReplicationType type, HddsProtos.ReplicationFactor factor,
String owner) throws IOException {
+ ReplicationConfig replicationConfig =
+ ReplicationConfig.fromProtoTypeAndFactor(type, factor);
+ return allocateContainer(replicationConfig, owner);
+ }
- ContainerRequestProto request = ContainerRequestProto.newBuilder()
- .setTraceID(TracingUtil.exportCurrentSpan())
- .setReplicationFactor(factor)
- .setReplicationType(type)
- .setOwner(owner)
- .build();
+ @Override
+ public ContainerWithPipeline allocateContainer(
+ ReplicationConfig replicationConfig, String owner) throws IOException {
+
+ ContainerRequestProto request;
+ if (replicationConfig.getReplicationType() ==
HddsProtos.ReplicationType.EC) {
+ HddsProtos.ECReplicationConfig ecProto = ((ECReplicationConfig)
replicationConfig).toProto();
+ request = ContainerRequestProto.newBuilder()
+ .setTraceID(TracingUtil.exportCurrentSpan())
+ .setEcReplicationConfig(ecProto)
+ .setReplicationFactor(ReplicationFactor.ONE)// Set for backward
compatibility, ignored for EC.
+ .setReplicationType(replicationConfig.getReplicationType())
+ .setOwner(owner)
+ .build();
+ } else {
+ request = ContainerRequestProto.newBuilder()
+ .setTraceID(TracingUtil.exportCurrentSpan())
+
.setReplicationFactor(ReplicationFactor.valueOf(replicationConfig.getReplication()))
+ .setReplicationType(replicationConfig.getReplicationType())
+ .setOwner(owner)
+ .build();
+ }
Review Comment:
Please reduce duplication. Builder can be created, traceID and owner can be
set regardless of replication config. `builder.setContainerRequest` accepts
both builder and proto, so you don't even need to call `build()` at the end.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java:
##########
@@ -244,7 +250,7 @@ public ContainerWithPipeline
allocateContainer(HddsProtos.ReplicationType
getScm().checkAdminAccess(getRemoteUser(), false);
final ContainerInfo container = scm.getContainerManager()
.allocateContainer(
- ReplicationConfig.fromProtoTypeAndFactor(replicationType,
factor),
+ replicationConfig,
owner);
Review Comment:
nit: Line wrap is no longer needed.
```java
.allocateContainer(replicationConfig, owner);
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestContainerOperations.java:
##########
@@ -265,4 +267,25 @@ public void testNodeOperationalStates() throws Exception {
nm.setNodeOperationalState(node, originalState);
}
}
+
+ @Test
+ public void testCreateRatis() throws Exception {
+ ContainerWithPipeline container = storageClient.createContainer(
+
ReplicationConfig.fromProtoTypeAndFactor(HddsProtos.ReplicationType.RATIS,
HddsProtos.ReplicationFactor.THREE),
Review Comment:
Please use `RatisReplicationConfig.getInstance`. (Also in
`TestAllocateContainer`.)
--
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]
