Skip to content

HDDS-5047. Refactor Pipeline to use ReplicationConfig instead of factor/type #2096

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
elek merged 12 commits into from
Apr 22, 2021
Merged

HDDS-5047. Refactor Pipeline to use ReplicationConfig instead of factor/type #2096

elek merged 12 commits into from
Apr 22, 2021

Conversation

@elek
Copy link
Member

@elek elek commented Mar 30, 2021

What changes were proposed in this pull request?

HDDS-5011 introduces Java ReplicationConfig classes which can be used as a replacement of replicationType and replicationFactor.

First task is replacing type/factor with ReplicationConfig in Pipeline and related managers (PipelineManager BackgroundPipelineCreatorV2, PipelineStateManager...)

We can do it on the master without the EC related stuff... (later we will add the small part which is required for EC...)

What is the link to the Apache JIRA

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

How was this patch tested?

Main logic is not changed, just the signature of the interfaces (and implementations) all the related unit tests and integration tests are updated.

@elek
Copy link
Member Author

elek commented Mar 30, 2021

This pr depends on #2089

bshashikant
bshashikant reviewed Apr 8, 2021
View reviewed changes
Copy link
Contributor

@bshashikant bshashikant left a comment

Choose a reason for hiding this comment

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

The approach looks good to me. I have one question regarding compatibility:

  1. How does a new client can still work with old server?

@elek
Copy link
Member Author

elek commented Apr 8, 2021

How does a new client can still work with old server?

Good question. The trick here is that we don't do any protobuf modifications. We serialize the protobuf content to a new Java object structure which makes it possible to handle additional parameters (for example: EC can handle more properties instead of just the one replicationFactor).

image

This patch modifies the SCM to use replicationConfig everywhere, but the logic is the same as before. With this approach on the EC branch we can add a new replicationConfig implementation and all the pipeline managers will serve it.

@elek
Copy link
Member Author

elek commented Apr 8, 2021

How does a new client can still work with old server?

Sorry, I didn't answer this exact question: there is no new client here. But EC client will add additional field and can work with old server as soon as EC replication type is not used.

So everything will work as before.

sodonnel
sodonnel reviewed Apr 12, 2021
View reviewed changes
...dds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java Outdated
.hasFactor(replicationConfig, ReplicationFactor.ONE)
&& autoCreateRatisOne;
boolean bgCreateThree = RatisReplicationConfig
.hasFactor(replicationConfig, ReplicationFactor.ONE);
Copy link
Contributor

@sodonnel sodonnel Apr 12, 2021

Choose a reason for hiding this comment

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

Is this last line of the change a mistake? The original code had THREE but this last line has ONE.

Copy link
Member Author

@elek elek Apr 14, 2021

Choose a reason for hiding this comment

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

yep. Thanks, fixing.

sodonnel
sodonnel reviewed Apr 12, 2021
View reviewed changes
hadoop-hdds/dev-support/checkstyle/checkstyle.xml

<module name="LineLength">
<property name="fileExtensions" value="java"/>
</module>
Copy link
Contributor

@sodonnel sodonnel Apr 12, 2021

Choose a reason for hiding this comment

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

Is this change intentional?

Copy link
Member Author

@elek elek Apr 14, 2021

Choose a reason for hiding this comment

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

Nope. My intellij removed it. Let me restore.

@sodonnel
Copy link
Contributor

sodonnel commented Apr 12, 2021

@elek Thanks for this large and repetitive change. I think it is needed to enable EC to progress more easily.

This change looks mostly good to me. I have just a few comments inline for you to double check. If we address those I am +1.

@elek
Copy link
Member Author

elek commented Apr 14, 2021 •
edited
Loading

Thank you very much the review/comments @sodonnel. Especially as I know how ungrateful is reviewing such a big task.

Fixes are pushed in a576fab

umamaheswararao
umamaheswararao reviewed Apr 14, 2021
View reviewed changes
.../server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/BackgroundPipelineCreator.java
// For STAND_ALONE Replication Type, Replication Factor 3 should not be
// used.
return factor == HddsProtos.ReplicationFactor.THREE;
return ((StandaloneReplicationConfig) replicationConfig)
Copy link
Contributor

@umamaheswararao umamaheswararao Apr 14, 2021

Choose a reason for hiding this comment

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

factor != ONE make sense here?

Copy link
Member Author

@elek elek Apr 22, 2021

Choose a reason for hiding this comment

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

I tried to avoid modification in the algorithm, but I am fine with that. Just modified in d8d038a

...ds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocol/ScmBlockLocationProtocol.java
* @return allocated block accessing info (key, pipeline).
* @throws IOException
*/
List<AllocatedBlock> allocateBlock(long size, int numBlocks,
Copy link
Contributor

@umamaheswararao umamaheswararao Apr 14, 2021

Choose a reason for hiding this comment

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

Why owner fix included in this patch?

Copy link
Member Author

@elek elek Apr 22, 2021

Choose a reason for hiding this comment

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

Only javadoc fixes are included. Here it is showed only because I modified ReplicationConfig at the same line:

Old code

       ReplicationType type, ReplicationFactor factor, String owner,

New code:

       ReplicationType type, ReplicationFactor factor, String owner,

sodonnel
sodonnel approved these changes Apr 14, 2021
View reviewed changes
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.

Latest version LGTM

@umamaheswararao umamaheswararao self-requested a review April 21, 2021 15:51
umamaheswararao
umamaheswararao approved these changes Apr 21, 2021
View reviewed changes
Copy link
Contributor

@umamaheswararao umamaheswararao left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@elek
Copy link
Member Author

elek commented Apr 22, 2021

Merging it now. Thanks the review @sodonnel and @umamaheswararao

@elek elek merged commit fb3dee8 into apache:master Apr 22, 2021
@elek elek mentioned this pull request May 3, 2021
Merged
errose28 added a commit to errose28/ozone that referenced this pull request May 4, 2021
@errose28
Merge remote-tracking branch 'upstream/master' into HDDS-3698-nonroll…
253cc55 
…ing-upgrade-master-merge2

* upstream/master: (56 commits)
  HDDS-2212. Genconf tool should generate config files for secure clust… (apache#1788)
  HDDS-5166. Remove duplicate assignment of OZONE_OPTS for freon and sh (apache#2195)
  Revert "HDDS-5144. Create github check to alert when dependency tree is changed (apache#2177)"
  HDDS-4983. Display key offset for each block in command key info (apache#2051)
  HDDS-5144. Create github check to alert when dependency tree is changed (apache#2177)
  HDDS-4585. Support bucket acl operation in S3g (apache#1701)
  HDDS-5153. Decommissioning a dead node should complete immediately (apache#2190)
  HDDS-5147. Intermittent test failure in TestContainerDeletionChoosingPolicy#testRandomChoosingPolicy (apache#2188)
  HDDS-5152. Fix Suggested leader in Client. (apache#2189)
  HDDS-5148. Bump ratis version to 2.1.0-ff8aa66-SNAPSHOT (apache#2184)
  HDDS-4515. Datanodes should be able to persist and load CRL (apache#2181)
  HDDS-5060. [SCM HA Security] Make InterSCM grpc channel secure. (apache#2187)
  HDDS-5051. Ensure failover to suggested leader if any for NotLeaderException. (apache#2141)
  HDDS-5127. Fix getServiceList when SCM HA is enabled (apache#2173)
  HDDS-4889. Add simple CI check for docs (apache#2156)
  HDDS-5131. Use timeout in github actions (apache#2176)
  HDDS-5103. Fix Install Snapshot Mechanism in SCMStateMachine. (apache#2155)
  HDDS-5124. Use OzoneConsts.OZONE_TIME_ZONE instead of "GMT" (apache#2166)
  HDDS-5047. Refactor Pipeline to use ReplicationConfig instead of factor/type (apache#2096)
  HDDS-5083. Bump version of common-compress (apache#2139)
  ...

Conflicts:
	hadoop-hdds/common/pom.xml
	hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
	hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java
	hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java
	hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManager.java
	hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMStorageConfig.java
	hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
	hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java
	hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java
	hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMStorage.java
	hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
	hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconStorageContainerManagerFacade.java
errose28 added a commit to errose28/ozone that referenced this pull request May 13, 2021
@errose28
Merge branch 'HDDS-3698-nonrolling-upgrade' into HDDS-5138-admin-chec…
080019b 
…k-in-auth

* HDDS-3698-nonrolling-upgrade: (57 commits)
  Fix compilation errors afte merge Update javassist in recon pom Fix changes introduced in merge that failed TestSCMNodeManager upgrade tests Fix checkstyle Fix intermittent test failure TestSCMNodeManager#testSetNodeOpStateAndCommandFired after merge Skip scm init default layout version in TestOzoneConfigurationFields
  HDDS-2212. Genconf tool should generate config files for secure clust… (apache#1788)
  HDDS-5166. Remove duplicate assignment of OZONE_OPTS for freon and sh (apache#2195)
  Revert "HDDS-5144. Create github check to alert when dependency tree is changed (apache#2177)"
  HDDS-4983. Display key offset for each block in command key info (apache#2051)
  HDDS-5144. Create github check to alert when dependency tree is changed (apache#2177)
  HDDS-4585. Support bucket acl operation in S3g (apache#1701)
  HDDS-5153. Decommissioning a dead node should complete immediately (apache#2190)
  HDDS-5147. Intermittent test failure in TestContainerDeletionChoosingPolicy#testRandomChoosingPolicy (apache#2188)
  HDDS-5152. Fix Suggested leader in Client. (apache#2189)
  HDDS-5148. Bump ratis version to 2.1.0-ff8aa66-SNAPSHOT (apache#2184)
  HDDS-4515. Datanodes should be able to persist and load CRL (apache#2181)
  HDDS-5060. [SCM HA Security] Make InterSCM grpc channel secure. (apache#2187)
  HDDS-5051. Ensure failover to suggested leader if any for NotLeaderException. (apache#2141)
  HDDS-5127. Fix getServiceList when SCM HA is enabled (apache#2173)
  HDDS-4889. Add simple CI check for docs (apache#2156)
  HDDS-5131. Use timeout in github actions (apache#2176)
  HDDS-5103. Fix Install Snapshot Mechanism in SCMStateMachine. (apache#2155)
  HDDS-5124. Use OzoneConsts.OZONE_TIME_ZONE instead of "GMT" (apache#2166)
  HDDS-5047. Refactor Pipeline to use ReplicationConfig instead of factor/type (apache#2096)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bshashikant bshashikant bshashikant left review comments

@umamaheswararao umamaheswararao umamaheswararao approved these changes

@sodonnel sodonnel sodonnel approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@elek @sodonnel @umamaheswararao @bshashikant