Skip to content

Conversation

@ghuangups
Copy link
Contributor

@ghuangups ghuangups commented Mar 29, 2021

… information.

What changes were proposed in this pull request?

"ozone admin scm roles" command currently returns scm role info like this: [scm1:9865, scm2:9865, scm3:9865]. This change is to add: LEADER and FOLLOWER info in the return. The new result from the command is now look like this: [scm3:9865:FOLLOWER, scm2:9865:FOLLOWER, scm1:9865:LEADER]

What is the link to the Apache JIRA

(Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HDDS-XXXX. Fix a typo in YYY.)

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

How was this patch tested?

Manually tested locally by creating a SCM HA cluster with 3 SCM nodes and executed command: ozone admin scm roles from one of the SCM node. With this change, the command result was: [scm3:9865:FOLLOWER, scm2:9865:FOLLOWER, scm1:9865:LEADER].

Copy link
Contributor

@avijayanhwx avijayanhwx left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @ghuangups. Left a comment inline.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if we should rely on the getAdminAddress() call to be null to determine if this node is LEADER or FOLLOWER. Can we do something similar to OM? The SCM that is returning the answer will be leader (since read calls go only to Leader), and the other nodes will be followers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can we use org.apache.ratis.proto.RaftProtos.RaftPeerRole#LEADER/FOLLOWER instead of hardcoded strings?

Copy link
Contributor

@GlenGeng-awx GlenGeng-awx Mar 30, 2021

Choose a reason for hiding this comment

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

  private static RaftPeer.Builder raftPeerBuilderFor(DatanodeDetails dn) {
    return RaftPeer.newBuilder()
        .setId(toRaftPeerId(dn))
        .setAddress(toRaftPeerAddress(dn, Port.Name.RATIS_SERVER))
        .setAdminAddress(toRaftPeerAddress(dn, Port.Name.RATIS_ADMIN))
        .setClientAddress(toRaftPeerAddress(dn, Port.Name.RATIS));
  }

I consider that each raft peer has its own admin address, no matter if it is leader or follower.

Better use NetUtils.isLocalAddress() to determine the peer whose address equals local, and assign him the leader role. Since SCM will do leader check before handle this request, the local SCM will be the leader.

@bshashikant bshashikant changed the base branch from HDDS-2823 to master March 30, 2021 07:15
@ghuangups
Copy link
Contributor Author

Somehow the repo rejected my push. I had to use -f to overwrite. Created a new PR: #2098.

@ghuangups ghuangups closed this Mar 31, 2021
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.

3 participants