[
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/14/22, 8:10 PM:
---------------------------------------------------------------------
||*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.\||
|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|NO|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|https://issues.apache.org/jira/browse/HDDS-6308|
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*|
|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.\|
|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.
> 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-unsubscr...@ozone.apache.org
For additional commands, e-mail: issues-h...@ozone.apache.org
