[
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: [email protected]
For additional commands, e-mail: [email protected]
