adoroszlai commented on code in PR #8559: URL: https://github.com/apache/ozone/pull/8559#discussion_r2125760204
########## hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ContainerReplicationOptions.java: ########## @@ -0,0 +1,42 @@ +/* + * 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.scm.cli.container; + +import org.apache.hadoop.ozone.shell.ReplicationOptions; +import picocli.CommandLine; + +/** + * CLI options for specifying container replication type and replication definition. + */ + +public class ContainerReplicationOptions extends ReplicationOptions { Review Comment: Why not use `ShellReplicationOptions` instead of duplicating it? ########## hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java: ########## @@ -261,6 +261,41 @@ public ContainerWithPipeline allocateContainer(HddsProtos.ReplicationType } } + @Override + public ContainerWithPipeline allocateContainer(ReplicationConfig replicationConfig, String owner) throws IOException { + Map<String, String> auditMap = Maps.newHashMap(); + auditMap.put("replicationType", String.valueOf(replicationConfig.getReplicationType())); + if (replicationConfig.getReplicationType() == HddsProtos.ReplicationType.RATIS || + replicationConfig.getReplicationType() == HddsProtos.ReplicationType.STAND_ALONE) { + auditMap.put("factor", String.valueOf(replicationConfig.getReplication())); + } + auditMap.put("owner", String.valueOf(owner)); + + try { + if (scm.getScmContext().isInSafeMode()) { + throw new SCMException("SafeModePrecheck failed for allocateContainer", + ResultCodes.SAFE_MODE_EXCEPTION); + } + getScm().checkAdminAccess(getRemoteUser(), false); + final ContainerInfo container = scm.getContainerManager() + .allocateContainer( + replicationConfig, + owner); + final Pipeline pipeline = scm.getPipelineManager() + .getPipeline(container.getPipelineID()); + ContainerWithPipeline cp = new ContainerWithPipeline(container, pipeline); + AUDIT.logWriteSuccess(buildAuditMessageForSuccess( + SCMAction.ALLOCATE_CONTAINER, auditMap) + ); + return cp; + } catch (Exception ex) { + AUDIT.logWriteFailure(buildAuditMessageForFailure( + SCMAction.ALLOCATE_CONTAINER, auditMap, ex) + ); + throw ex; + } Review Comment: Please avoid duplication. Extract a method for this, and let existing `allocateContainer` call it with `ReplicationConfig.fromProtoTypeAndFactor(replicationType, factor)`. ########## hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerOperationClient.java: ########## @@ -221,6 +221,33 @@ public ContainerWithPipeline createContainer(HddsProtos.ReplicationType type, } } + @Override + public ContainerWithPipeline createContainer(ReplicationConfig replicationConfig, + String owner) throws IOException { + XceiverClientSpi client = null; + XceiverClientManager clientManager = getXceiverClientManager(); + try { + if (replicationConfig == null) { + replicationConfig = ReplicationConfig.fromProtoTypeAndFactor(replicationType, replicationFactor); + } + // allocate container on SCM. + ContainerWithPipeline containerWithPipeline = + storageContainerLocationClient.allocateContainer(replicationConfig, + owner); + Pipeline pipeline = containerWithPipeline.getPipeline(); + // connect to pipeline leader and allocate container on leader datanode. + client = clientManager.acquireClient(pipeline); + createContainer(client, + containerWithPipeline.getContainerInfo().getContainerID()); + return containerWithPipeline; + } finally { + if (client != null) { + clientManager.releaseClient(client, false); + } + } Review Comment: Please avoid duplicating logic from existing method. ########## hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerOperationClient.java: ########## @@ -221,6 +221,33 @@ public ContainerWithPipeline createContainer(HddsProtos.ReplicationType type, } } + @Override + public ContainerWithPipeline createContainer(ReplicationConfig replicationConfig, + String owner) throws IOException { Review Comment: nit: Please do not format method signature like this. Whenever visibility / return type / method name / other modifiers are changed, we would have to reindent all parameters. ########## hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerOperationClient.java: ########## @@ -221,6 +221,33 @@ public ContainerWithPipeline createContainer(HddsProtos.ReplicationType type, } } + @Override + public ContainerWithPipeline createContainer(ReplicationConfig replicationConfig, + String owner) throws IOException { + XceiverClientSpi client = null; + XceiverClientManager clientManager = getXceiverClientManager(); + try { + if (replicationConfig == null) { + replicationConfig = ReplicationConfig.fromProtoTypeAndFactor(replicationType, replicationFactor); + } Review Comment: Use `Objects.requireNonNull` to verify `replicationConfig` and avoid fallback to type/factor from config. ########## hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java: ########## @@ -261,6 +261,41 @@ public ContainerWithPipeline allocateContainer(HddsProtos.ReplicationType } } + @Override + public ContainerWithPipeline allocateContainer(ReplicationConfig replicationConfig, String owner) throws IOException { + Map<String, String> auditMap = Maps.newHashMap(); + auditMap.put("replicationType", String.valueOf(replicationConfig.getReplicationType())); + if (replicationConfig.getReplicationType() == HddsProtos.ReplicationType.RATIS || + replicationConfig.getReplicationType() == HddsProtos.ReplicationType.STAND_ALONE) { + auditMap.put("factor", String.valueOf(replicationConfig.getReplication())); + } Review Comment: ```suggestion auditMap.put("replication", String.valueOf(replicationConfig.getReplication())); ``` ########## hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto: ########## @@ -209,6 +221,72 @@ message ContainerRequestProto { optional string traceID = 5; } +message ECContainerRequestProto { + required ECReplicationConfig ecReplicationConfig = 1; + required string owner = 2; + optional string traceID = 3; +} + +message ECContainerResponseProto { + enum Error { + success = 1; + errorContainerAlreadyExists = 2; + errorContainerMissing = 3; + scmNotLeader = 4; + } + required Error errorCode = 1; + required ContainerWithPipeline containerWithPipeline = 2; + optional string errorMessage = 3; +} + +message GetEcContainerRequestProto { + required int64 containerID = 1; + optional string traceID = 2; + +} + +message GetEcContainerResponseProto { + required ContainerInfoProto containerInfo = 1; +} + +message GetEcContainerWithPipelineRequestProto { + required int64 containerID = 1; + optional string traceID = 2; + +} + +message GetEcContainerWithPipelineResponseProto { + required ContainerWithPipeline containerWithPipeline = 1; +} + +message GetEcContainerReplicasRequestProto { + required int64 containerID = 1; + optional string traceID = 2; +} + +message GetEcContainerReplicasResponseProto { + repeated SCMContainerReplicaProto containerReplica = 1; +} + +message GetEcContainerWithPipelineBatchRequestProto { + repeated int64 containerIDs = 1; + optional string traceID = 2; +} + +message GetExistEcContainerWithPipelinesInBatchResponseProto { + repeated int64 containerIDs = 1; + optional string traceID = 2; +} + +message GetExistEcContainerWithPipelinesInBatchRequestProto { + repeated int64 containerIDs = 1; + optional string traceID = 2; +} + +message GetEcContainerWithPipelineBatchResponseProto { + repeated ContainerWithPipeline containerWithPipelines = 1; +} + Review Comment: I don't think these new requests and responses are needed. Add `optional ECReplicationConfig ecReplicationConfig` to existing `ContainerRequestProto`, and use `hasEcReplicationConfig()` to decide if it's EC or not. ########## hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/client/ScmClient.java: ########## @@ -179,10 +179,14 @@ ContainerDataProto readContainer(long containerID) * @return ContainerInfo * @throws IOException - in case of error. */ + @Deprecated ContainerWithPipeline createContainer(HddsProtos.ReplicationType type, HddsProtos.ReplicationFactor replicationFactor, String owner) throws IOException; + ContainerWithPipeline createContainer(ReplicationConfig replicationConfig, + String owner) throws IOException; Review Comment: nit: Please do not format method signature like this. Whenever visibility / return type / method name / other modifiers are changed, we would have to reindent all parameters. ########## hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java: ########## @@ -243,6 +246,51 @@ public ContainerWithPipeline allocateContainer( response.getContainerWithPipeline()); } + @Override + public ContainerWithPipeline allocateContainer( + ReplicationConfig replicationConfig, String owner) throws IOException { + + if (replicationConfig.getReplicationType() == RATIS || replicationConfig.getReplicationType() == STAND_ALONE) { + ContainerRequestProto request = ContainerRequestProto.newBuilder() + .setTraceID(TracingUtil.exportCurrentSpan()) + .setReplicationFactor(ReplicationFactor.valueOf(replicationConfig.getReplication())) + .setReplicationType(replicationConfig.getReplicationType()) + .setOwner(owner) + .build(); + + ContainerResponseProto response = + submitRequest(Type.AllocateContainer, + builder -> builder.setContainerRequest(request)) + .getContainerResponse(); + //TODO should be migrated to use the top level status structure. + if (response.getErrorCode() != ContainerResponseProto.Error.success) { + throw new IOException(response.hasErrorMessage() ? + response.getErrorMessage() : "Allocate container failed."); + } + return ContainerWithPipeline.fromProtobuf( + response.getContainerWithPipeline()); Review Comment: Please avoid duplication, let existing `allocateContainer` call the new method. -- 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-unsubscr...@ozone.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org For additional commands, e-mail: issues-h...@ozone.apache.org
