bharatviswa504 commented on a change in pull request #2155:
URL: https://github.com/apache/ozone/pull/2155#discussion_r612945645
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java
##########
@@ -101,14 +101,19 @@ public void start() throws IOException {
if (ratisServer.getDivision().getGroup().getPeers().isEmpty()) {
// this is a bootstrapped node
// It will first try to add itself to existing ring
- boolean success = HAUtils.addSCM(OzoneConfiguration.of(conf),
+ final SCMNodeDetails details =
+ scm.getSCMHANodeDetails().getLocalNodeDetails();
+ final boolean success = HAUtils.addSCM(OzoneConfiguration.of(conf),
new AddSCMRequest.Builder().setClusterId(scm.getClusterId())
.setScmId(scm.getScmId())
.setRatisAddr(scm.getSCMHANodeDetails().getLocalNodeDetails()
Review comment:
Minor Nit:
We can use details.getRatisHostPortStr() instead of
scm.getSCMHANodeDetails().getLocalNodeDetails().getRatisHostPortStr()
And also can we rename details -> nodeDetails
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/InterSCMGrpcClient.java
##########
@@ -52,14 +52,8 @@
private final InterSCMProtocolServiceGrpc.InterSCMProtocolServiceStub
client;
- private final long timeout;
-
- public InterSCMGrpcClient(final String host, final ConfigurationSource conf)
{
- Preconditions.checkNotNull(conf);
- int port = conf.getInt(ScmConfigKeys.OZONE_SCM_GRPC_PORT_KEY,
- ScmConfigKeys.OZONE_SCM_GRPC_PORT_DEFAULT);
- timeout =
- conf.getObject(SCMHAConfiguration.class).getGrpcDeadlineInterval();
+ public InterSCMGrpcClient(final String host, final int port,
+ final long timeout) {
Review comment:
Can we retain passing in conf also, as it will be required for
HDDS-5060, as it needs to create SecurityConfig from conf and enable sslContext.
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMStateMachine.java
##########
@@ -185,10 +186,16 @@ public void
notifyNotLeader(Collection<TransactionContext> pendingEntries) {
public CompletableFuture<TermIndex> notifyInstallSnapshotFromLeader(
RaftProtos.RoleInfoProto roleInfoProto, TermIndex firstTermIndexInLog) {
- String leaderNodeId = RaftPeerId.valueOf(roleInfoProto.getFollowerInfo()
- .getLeaderInfo().getId().getId()).toString();
+ String leaderNode = roleInfoProto.getFollowerInfo()
+ .getLeaderInfo().getId().getAddress();
+ Optional<SCMNodeDetails> leaderDetails =
+ scm.getSCMHANodeDetails().getPeerNodeDetails().stream().filter(
+ p -> p.getRatisHostPortStr().equals(p.getRatisHostPortStr()))
Review comment:
NIT: Can we rename that to leaderAddress?
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMStateMachine.java
##########
@@ -185,10 +186,16 @@ public void
notifyNotLeader(Collection<TransactionContext> pendingEntries) {
public CompletableFuture<TermIndex> notifyInstallSnapshotFromLeader(
RaftProtos.RoleInfoProto roleInfoProto, TermIndex firstTermIndexInLog) {
- String leaderNodeId = RaftPeerId.valueOf(roleInfoProto.getFollowerInfo()
- .getLeaderInfo().getId().getId()).toString();
+ String leaderNode = roleInfoProto.getFollowerInfo()
+ .getLeaderInfo().getId().getAddress();
+ Optional<SCMNodeDetails> leaderDetails =
+ scm.getSCMHANodeDetails().getPeerNodeDetails().stream().filter(
+ p -> p.getRatisHostPortStr().equals(p.getRatisHostPortStr()))
Review comment:
Should this be here compared it with leaderNode above where we get
leaderAddress?
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMStateMachine.java
##########
@@ -185,10 +186,16 @@ public void
notifyNotLeader(Collection<TransactionContext> pendingEntries) {
public CompletableFuture<TermIndex> notifyInstallSnapshotFromLeader(
RaftProtos.RoleInfoProto roleInfoProto, TermIndex firstTermIndexInLog) {
- String leaderNodeId = RaftPeerId.valueOf(roleInfoProto.getFollowerInfo()
- .getLeaderInfo().getId().getId()).toString();
+ String leaderNode = roleInfoProto.getFollowerInfo()
Review comment:
In tests I see leaderNode coming ""
Received install snapshot notification from SCM leader: with term index:
(t:2, i:107)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
