Skip to content

Conversation

@sadanand48
Copy link
Contributor

@sadanand48 sadanand48 commented Feb 28, 2022

What changes were proposed in this pull request?

Current code for #getScmRatisRoles calls SCMRatisServer.getRatisRoles() which basically determines the leader based on whether the node is local.
peer.getAddress().concat(isLocal ? ":".concat(RaftProtos.RaftPeerRole.LEADER.toString()) : ":".concat(RaftProtos.RaftPeerRole.FOLLOWER.toString()))
This logic makes sense for other cases since only leader will process this request, but not for jmx case. The web ui of a particular scm will show itself as the leader according to this logic which is incorrect. Using scmContext.isLeader() instead to solve this

What is the link to the Apache JIRA

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

How was this patch tested?

Tested on docker scm ha env
Screenshot 2022-03-01 at 12 50 29 AM

Screenshot 2022-03-01 at 12 56 23 AM

@sadanand48 sadanand48 requested a review from nandakumar131 March 2, 2022 14:02
sb.append(String
.format("{ HostName : %s, Ratis Port : %s, Role : %s } ", x[0], x[1],
x[2]));
.format("{ HostName : %s, Ratis Port : %s } ", x[0], x[1]));
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @sadanand48 for reporting this issue.

This string format is not only used on Web UI, but also the CLI scm roles command. So I would suggest keep the Role info here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChenSammi Thanks for the review. The format method here is only used in StorageContainerManager#getRatisRoles which is an implementation of the SCMMXBean (Web UI)
In case of the ozone admin scm getroles command, it creates an SCMClient and calls SCMClient#getRatisRoles which creates an scm client and does an RPC which is executed only by leader.
The SCMContext of an SCM only knows whether itself is a leader or not, If its not a leader, then we don’t know which one is the leader without doing an RPC.

@ChenSammi
Copy link
Contributor

@sadanand48 , can we just change the logic in #getRatisRoles, using the SCMContext.isLeader to identity the ROLE INFO of each peer instead?

@adoroszlai
Copy link
Contributor

@sadanand48 Can we wrap this up?

@sadanand48
Copy link
Contributor Author

I guess the approach in #3401 is the goto approach in case we want to display every SCM Role in UI. The current appoach would only tell whether itself is a leader. If we decide to go with that approach, I can close this PR and we can reopen #3144
cc @symious

@adoroszlai
Copy link
Contributor

I guess the approach in #3401 is the goto approach in case we want to display every SCM Role in UI. The current appoach would only tell whether itself is a leader.

Thanks. Closing in favor of #3401.

@adoroszlai adoroszlai closed this Jul 25, 2022
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