[
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 edited comment on HDDS-6254 at 2/10/22, 1:28 AM:
---------------------------------------------------------------------
||*Class Name*|*Need Fix ?*|*Review Comments*|
|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.|
|ReplicationConfig.java|NO|This is the class exposed that API.|
|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.|
|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.|
|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|
|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.|
|OzoneMultipartUpload.java|YES| S3 Storage Types currently not supported EC
stuff. We need to fix this.|
|OzoneMultipartUploadPartListParts.java|YES|Same as above|
|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.|
|OmKeyInfo.java| |if (replicationConfig instanceof ECReplicationConfig) \{
kb.setEcReplicationConfig( ((ECReplicationConfig)
replicationConfig).toProto()); }else{
kb.setFactor(ReplicationConfig.getLegacyFactor(replicationConfig)); }|
|OmMultipartKeyInfo.java|YES|This may need quite a few changes as we need to
introduce ECReplicationConfig as part of proto files etc for MultipartKeyInfo|
|OzoneManagerProtocolClientSideTranslatorPB.java|YES|initiateMultipartUpload
needs the fix|
|TestContainerStateManagerIntegration.java| | |
|TestOzoneAtRestEncryption.java| | |
|TestOzoneRpcClientAbstract.java| | |
|TestSecureOzoneRpcClient.java| | |
|TestDataScrubber.java| | |
|OzoneManagerRequestHandler.java|YES|Needs fixes for Multipart upload related|
|NodeEndpoint.java|FIXED|https://issues.apache.org/jira/browse/HDDS-6272|
|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.
was (Author: umamaheswararao):
||*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]
