Skip to content

Conversation

@adoroszlai
Copy link
Contributor

What changes were proposed in this pull request?

Avoid unnecessarily using getLegacyFactory, to make Datanode Chunk Validator work even if there are EC pipelines in the cluster.

https://issues.apache.org/jira/browse/HDDS-6252

How was this patch tested?

$ ozone sh volume create /vol1

# create an EC pipeline
$ ozone sh bucket create --replication rs-3-2-1024k --type EC /vol1/becket
$ ozone sh key put /vol1/becket/passwd /etc/passwd

# test DCG/DCV
$ echo log4j.logger.org.apache.hadoop.ozone.freon=DEBUG >> /etc/hadoop/log4j.properties
$ ozone freon dcg -t1 -n100 -p dcg
...
$ ozone freon dcv -t1 -n100 -p dcg
...
[main] DEBUG freon.DatanodeChunkValidator: Found pipeline PipelineID=47a983b3-6b22-4330-9406-75601aeba8e1
...
[main] INFO freon.DatanodeChunkValidator: Using pipeline PipelineID=28f4f444-a174-4607-8e9e-060339a8801b
...
Successful executions: 100

# check ID of EC pipeline (compare to output of dcv)
$ ozone admin pipeline list | grep 'ReplicationConfig: EC'
Pipeline[ Id: 47a983b3-6b22-4330-9406-75601aeba8e1, ...

CI (with no EC pipeline yet) still OK:
https://github.com/adoroszlai/hadoop-ozone/runs/5042702809#step:5:669

@adoroszlai adoroszlai self-assigned this Feb 3, 2022
@adoroszlai adoroszlai requested a review from sodonnel February 3, 2022 06:18
.filter(
p -> ReplicationConfig.getLegacyFactor(p.getReplicationConfig())
== HddsProtos.ReplicationFactor.THREE)
.filter(p -> p.getReplicationConfig().getRequiredNodes() == 3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than "3" here, should we have "HddsProtos.ReplicationFactor.THREE.getValue()", so we are using the constant. It is so unlikely the definition of THREE will change so I am fine either way.

Copy link
Contributor Author

@adoroszlai adoroszlai Feb 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied this condition from the corresponding generator class:

.filter(p -> p.getReplicationConfig().getRequiredNodes() == 3)

But searching for this fragment I found that the base class has a utility method for the same purpose, so we can get rid of some duplication.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - sounds good to me. On a wider node, I reckon we need to check all usages of ReplicationConfig.getLegacyFactor(), as anywhere that gets called on a EC config, it will fail like this one does. I will raise another EC Jira for that task.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will raise another EC Jira for that task.

These are the files with getLegacyFactor (on EC branch), you can include the list in the task description:

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HDDS-6254 raised for the followup task.

Copy link
Contributor

@sodonnel sodonnel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after green CI.

@sodonnel sodonnel merged commit ddcfc52 into apache:HDDS-3816-ec Feb 3, 2022
@adoroszlai adoroszlai deleted the HDDS-6252 branch February 3, 2022 17:09
@adoroszlai
Copy link
Contributor Author

Thanks @sodonnel for reviewing and committing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants