[jira] [Commented] (HDDS-6254) EC: Review use of ReplicationConfig.getLegacyFactor() in the codebase

["Uma Maheswara Rao G (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HDDS-6254?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17489901#comment-17489901 ] 

Uma Maheswara Rao G commented on HDDS-6254:
-------------------------------------------

||*Class Name*|*Need Fix ?*|*Review Comments*|
|hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java|NO|if (pipeline.getType() != HddsProtos.ReplicationType.STAND_ALONE && pipeline
.getType() != HddsProtos.ReplicationType.EC) {
pipeline = Pipeline.newBuilder(pipeline)
.setReplicationConfig(new StandaloneReplicationConfig(
ReplicationConfig
.getLegacyFactor(pipeline.getReplicationConfig())))
.build();
}
 
This is safegarded by !=EC check. In the case of EC, it should not come to this code.|
|hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ReplicationConfig.java|NO|This is the class exposed that API.|
|hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java|NO|1. if (replicationConfig instanceof ECReplicationConfig) {
builder.setEcReplicationConfig(((ECReplicationConfig) replicationConfig)
.toProto());
} else {
builder.setReplicationFactor(
ReplicationConfig.getLegacyFactor(replicationConfig));
}
This is safe guarded.
2. @JsonIgnore
public HddsProtos.ReplicationFactor getReplicationFactor() {
return ReplicationConfig.getLegacyFactor(replicationConfig);
}
Invocation of this method is safe guarded already.|
|hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java|NO|if (replicationConfig instanceof ECReplicationConfig) {
builder.setEcReplicationConfig(((ECReplicationConfig) replicationConfig)
.toProto());
} else {
builder.setFactor(ReplicationConfig.getLegacyFactor(replicationConfig));
}
This is safe guarded.|
|hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java|NO|if (pipeline.getReplicationConfig() instanceof ECReplicationConfig) {
containerInfoBuilder.setEcReplicationConfig(
((ECReplicationConfig) pipeline.getReplicationConfig()).toProto());
} else {
containerInfoBuilder.setReplicationFactor(
ReplicationConfig.getLegacyFactor(pipeline.getReplicationConfig()));
}
This is also safe guarded.|
|hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java|?|if (state != null) {
if (factor != null) {
return scm.getContainerManager().getContainers(state).stream()
.filter(info -> info.containerID().getId() >= startContainerID)
//Filtering EC replication type as EC will not have factor.
.filter(info -> info
.getReplicationType() != HddsProtos.ReplicationType.EC)
.filter(info -> (info.getReplicationFactor() == factor))
.sorted().limit(count).collect(Collectors.toList());
} else {
return scm.getContainerManager().getContainers(state).stream()
.filter(info -> info.containerID().getId() >= startContainerID)
.sorted().limit(count).collect(Collectors.toList());
}
} else {
if (factor != null) {
return scm.getContainerManager().getContainers().stream()
.filter(info -> info.containerID().getId() >= startContainerID)
//Filtering EC replication type as EC will not have factor.
.filter(info -> info
.getReplicationType() != HddsProtos.ReplicationType.EC)
.filter(info -> info.getReplicationFactor() == factor)
.sorted().limit(count).collect(Collectors.toList());
} else {
return scm.getContainerManager().getContainers(containerId, count);
}
}
 
Probably we need to review this. Currently we just filtered if there are EC containers here. Assumption is that EC container should not have field factor. There are no direct usages, but is calling ContainerInfo#getReplicationFactor|
|hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/pipeline/ListPipelinesSubcommand.java|YES|if (!Strings.isNullOrEmpty(factor)) {
stream = stream.filter(
p -> ReplicationConfig.getLegacyFactor(p.getReplicationConfig())
.toString().compareToIgnoreCase(factor) == 0);
}
Currently it is provided only factor to filter. We should provide EC option to filter too.|
|hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneMultipartUpload.java|YES| S3 Storage Types currently not supported EC stuff. We need to fix this.|
|hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneMultipartUploadPartListParts.java|YES|Same as above|
|hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/checksum/ReplicatedFileChecksumHelper.java|?|This particular class is design for Replicated. SO, it's ok to call this API in this class. It may be good idea to safe guard where this class used.|
|hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java| |if (replicationConfig instanceof ECReplicationConfig) {
kb.setEcReplicationConfig(
((ECReplicationConfig) replicationConfig).toProto());
} else {
kb.setFactor(ReplicationConfig.getLegacyFactor(replicationConfig));
}|
|hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmMultipartKeyInfo.java|YES|This may need quite a few changes as we need to introduce ECReplicationConfig as part of proto files etc for MultipartKeyInfo|
|hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java|YES|initiateMultipartUpload needs the fix|
|hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerStateManagerIntegration.java| | |
|hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneAtRestEncryption.java| | |
|hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java| | |
|hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestSecureOzoneRpcClient.java| | |
|hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scrubber/TestDataScrubber.java| | |
|hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerRequestHandler.java|YES|Needs fixes for Multipart upload related|
|hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/NodeEndpoint.java|FIXED|https://issues.apache.org/jira/browse/HDDS-6272|
|hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/PipelineEndpoint.java|YES| |

I have reviewed the changes and created this Table. Some/most issues deserve separate JIRAs. Mainly we need to address MultipartUpload side. We have not done any work in that side.

> EC: Review use of ReplicationConfig.getLegacyFactor() in the codebase
> ---------------------------------------------------------------------
>
>                 Key: HDDS-6254
>                 URL: https://issues.apache.org/jira/browse/HDDS-6254
>             Project: Apache Ozone
>          Issue Type: Sub-task
>            Reporter: Stephen O'Donnell
>            Priority: Major
>
> When we call "ReplicationConfig.getLegacyFactor()" against an ECReplicationConfig object, it causes an UnsupportedOperation exception. This can happen in cases where list of pipelines are being filtered and an EC Pipeline is encountered. We need to review all usages of this and ensure they are safe. Current references to that method are in:
> {code:java}
> hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java
> hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/ReplicationConfig.java
> hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java
> hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java
> hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java
> hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
> hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/pipeline/ListPipelinesSubcommand.java
> hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneMultipartUpload.java
> hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneMultipartUploadPartListParts.java
> hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/checksum/ReplicatedFileChecksumHelper.java
> hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java
> hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmMultipartKeyInfo.java
> hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java
> hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerStateManagerIntegration.java
> hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneAtRestEncryption.java
> hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java
> hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestSecureOzoneRpcClient.java
> hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scrubber/TestDataScrubber.java
> hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerRequestHandler.java
> hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/NodeEndpoint.java
> hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/PipelineEndpoint.java {code}



--
This message was sent by Atlassian Jira
(v8.20.1#820001)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org