Skip to content

HDDS-6960. EC: Implement the Over-replication Handler #3572

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
JacksonYao287 merged 9 commits into from
Jul 5, 2022
Merged

HDDS-6960. EC: Implement the Over-replication Handler #3572

JacksonYao287 merged 9 commits into from
Jul 5, 2022

Conversation

@JacksonYao287
Copy link
Contributor

@JacksonYao287 JacksonYao287 commented Jun 30, 2022

What changes were proposed in this pull request?

Implement the Over-replication Handler

What is the link to the Apache JIRA

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

How was this patch tested?

unit test

umamaheswararao
umamaheswararao reviewed Jul 2, 2022
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.

Thank you @JacksonYao287 for working on this patch. I have few comments to check. Thanks

...src/main/java/org/apache/hadoop/hdds/scm/container/replication/ECOverReplicationHandler.java Outdated
try {
//the command target node should be in-service and healthy
return nodeManager.getNodeStatus(dd)
.equals(NodeStatus.inServiceHealthy());
Copy link
Contributor

@umamaheswararao umamaheswararao Jul 2, 2022

Choose a reason for hiding this comment

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

I think we discussed this. This is not covering healthy_readonly?

...rc/main/java/org/apache/hadoop/hdds/scm/container/replication/ECUnderReplicationHandler.java
* commands.
*/
public class ECUnderReplicationHandler implements UnderReplicationHandler {
public class ECUnderReplicationHandler implements UnhealthyReplicationHandler {
Copy link
Contributor

@umamaheswararao umamaheswararao Jul 2, 2022

Choose a reason for hiding this comment

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

Not sure we will call over replication as unhealthy. Don't have better name in my mind but how about simply ReplicationHandler ?

Since it is common class now, javadoc should represent the generic message. Currently it assumed for underReplication handler.

Copy link
Contributor Author

@JacksonYao287 JacksonYao287 Jul 3, 2022

Choose a reason for hiding this comment

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

ReplicationHandler seems a little confusing, it might be considered as the handler to handle replication command.
so i suggest we use unhealthy just for now and change it to an appropriate name after we find a proper one. what about "nonOptimalReplicationHandler"

@JacksonYao287
Copy link
Contributor Author

JacksonYao287 commented Jul 3, 2022

@umamaheswararao thanks for the review, will fix the comments later

umamaheswararao
umamaheswararao reviewed Jul 4, 2022
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.

Thanks @JacksonYao287 for updating the patch. Patch almost looks good to me, except a small comment left.

@JacksonYao287
Copy link
Contributor Author

JacksonYao287 commented Jul 4, 2022

Thanks @umamaheswararao for the review , i have update this patch according to your comments, pleas take a look

umamaheswararao
umamaheswararao reviewed Jul 4, 2022
View reviewed changes
...in/java/org/apache/hadoop/hdds/scm/container/replication/AbstractOverReplicationHandler.java
}
/**
* Identify a new set of datanode(s) to replicate/reconstruct the container
* and form the SCM commands to send it to DN.
Copy link
Contributor

@umamaheswararao umamaheswararao Jul 4, 2022

Choose a reason for hiding this comment

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

Missed to update doc?

Copy link
Contributor Author

@JacksonYao287 JacksonYao287 Jul 4, 2022

Choose a reason for hiding this comment

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

sorry for missing , fixed now!

...in/java/org/apache/hadoop/hdds/scm/container/replication/AbstractOverReplicationHandler.java Outdated
import java.util.stream.Collectors;

/**
* this class holds some common methods that will be shared among
Copy link
Contributor

@umamaheswararao umamaheswararao Jul 4, 2022

Choose a reason for hiding this comment

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

Typos: this --> This
Implementation -> implementation

umamaheswararao
umamaheswararao approved these changes Jul 4, 2022
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.

LGTM

@JacksonYao287
Copy link
Contributor Author

JacksonYao287 commented Jul 5, 2022

thanks @umamaheswararao for the review! i have committed this patch to master branch

@JacksonYao287 JacksonYao287 merged commit fac23c9 into apache:master Jul 5, 2022
@JacksonYao287 JacksonYao287 deleted the HDDS-6960 branch July 5, 2022 01:40
errose28 added a commit to errose28/ozone that referenced this pull request Jul 12, 2022
@errose28
Merge branch 'master' into HDDS-6761-take2
c9b9a9d 
* master: (46 commits)
  HDDS-6901. Configure HDDS volume reserved as percentage of the volume space. (apache#3532)
  HDDS-6978. EC: Cleanup RECOVERING container on DN restarts (apache#3585)
  HDDS-6982. EC: Attempt to cleanup the RECOVERING container when reconstruction failed at coordinator. (apache#3583)
  HDDS-6968. Addendum: [Multi-Tenant] Fix USER_MISMATCH error even on correct user. (apache#3578)
  HDDS-6794. EC: Analyze and add putBlock even on non writing node in the case of partial single stripe. (apache#3514)
  HDDS-6900. Propagate TimeoutException for all SCM HA Ratis calls. (apache#3564)
  HDDS-6938. handle NPE when removing prefixAcl (apache#3568)
  HDDS-6960. EC: Implement the Over-replication Handler (apache#3572)
  HDDS-6979. Remove unused plexus dependency declaration (apache#3579)
  HDDS-6957. EC: ReplicationManager - priortise under replicated containers (apache#3574)
  HDDS-6723. Close Rocks objects properly in OzoneManager (apache#3400)
  HDDS-6942. Ozone Buckets/Objects created via S3 should not allow group access (apache#3553)
  HDDS-6965. Increase timeout for basic check (apache#3563)
  HDDS-6969. Add link to compose directory in smoketest README (apache#3567)
  HDDS-6970. EC: Ensure DatanodeAdminMonitor can handle EC containers during decommission (apache#3573)
  HDDS-6977. EC: Remove references to ContainerReplicaPendingOps in TestECContainerReplicaCount (apache#3575)
  HDDS-6217. Cleanup XceiverClientGrpc TODOs, and document how the client works and should be used. (apache#3012)
  HDDS-6773. Cleanup TestRDBTableStore (apache#3434) - fix checkstyle
  HDDS-6773. Cleanup TestRDBTableStore (apache#3434)
  HDDS-6676. KeyValueContainerData#getProtoBufMessage() should set block count (apache#3371)
  ...

Conflicts:
    hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/upgrade/SCMUpgradeFinalizer.java
duongkame pushed a commit to duongkame/ozone that referenced this pull request Aug 16, 2022
@umamaheswararao
CDPD-41642: Backport Ozone EC JIRAs into CDPD
54b5b59 
HDDS-6960. EC: Implement the Over-replication Handler (apache#3572)

(cherry picked from commit fac23c9)
Change-Id: Ic273820549544731b781b143a810ee45ef0085df
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@umamaheswararao umamaheswararao umamaheswararao approved these changes

Assignees

No one assigned

Labels

EC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@JacksonYao287 @umamaheswararao