You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2023/01/19 06:23:05 UTC
[GitHub] [ratis] szetszwo opened a new pull request, #811: RATIS-1772. Refactor the startLeaderElection code in LeaderStateImpl.
szetszwo opened a new pull request, #811:
URL: https://github.com/apache/ratis/pull/811
See https://issues.apache.org/jira/browse/RATIS-1772
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #811: RATIS-1772. Refactor the startLeaderElection code in LeaderStateImpl.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #811:
URL: https://github.com/apache/ratis/pull/811#discussion_r1080870820
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -685,6 +685,40 @@ private synchronized void sendStartLeaderElectionToHigherPriorityPeer(RaftPeerId
});
}
+ boolean sendStartLeaderElection(FollowerInfo followerInfo, int leaderPriority) {
Review Comment:
We will get `FollowerInfo` (it is a must since we need match index) and then pass null for `leaderPriority`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #811: RATIS-1772. Refactor the startLeaderElection code in LeaderStateImpl.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #811:
URL: https://github.com/apache/ratis/pull/811#discussion_r1080871181
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -1024,49 +1057,15 @@ private long[] getSorted(List<FollowerInfo> followerInfos, boolean includeSelf,
return indices;
}
- private void yieldLeaderToHigherPriorityPeer() {
- if (!server.getInfo().isLeader()) {
- return;
- }
-
+ private void checkPeersForYieldingLeader() {
final RaftConfigurationImpl conf = server.getRaftConf();
final RaftPeer leader = conf.getPeer(server.getId());
if (leader == null) {
LOG.error("{} the leader {} is not in the conf {}", this, server.getId(), conf);
return;
}
- int leaderPriority = leader.getPriority();
-
for (LogAppender logAppender : senders.getSenders()) {
- final FollowerInfo followerInfo = logAppender.getFollower();
- final RaftPeerId followerID = followerInfo.getPeer().getId();
- final RaftPeer follower = conf.getPeer(followerID);
- if (follower == null) {
- if (conf.getPeer(followerID, RaftPeerRole.LISTENER) == null) {
- LOG.error("{} the follower {} is not in the conf {}", this, followerID, conf);
- }
- continue;
- }
- final int followerPriority = follower.getPriority();
- if (followerPriority <= leaderPriority) {
- continue;
- }
- final TermIndex leaderLastEntry = server.getState().getLastEntry();
- if (leaderLastEntry == null) {
- LOG.info("{} send StartLeaderElectionRequest to follower:{} on term:{} because follower's priority:{} " +
- "is higher than leader's:{} and leader's lastEntry is null",
- this, followerID, currentTerm, followerPriority, leaderPriority);
-
- sendStartLeaderElectionToHigherPriorityPeer(followerID, null);
- return;
- }
-
- if (followerInfo.getMatchIndex() >= leaderLastEntry.getIndex()) {
- LOG.info("{} send StartLeaderElectionRequest to follower:{} on term:{} because follower's priority:{} " +
- "is higher than leader's:{} and follower's lastEntry index:{} catch up with leader's:{}",
- this, followerID, currentTerm, followerPriority, leaderPriority, followerInfo.getMatchIndex(),
- leaderLastEntry.getIndex());
- sendStartLeaderElectionToHigherPriorityPeer(followerID, leaderLastEntry);
+ if (sendStartLeaderElection(logAppender.getFollower(), leader.getPriority())) {
Review Comment:
Sure, this is a good idea!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen commented on a diff in pull request #811: RATIS-1772. Refactor the startLeaderElection code in LeaderStateImpl.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #811:
URL: https://github.com/apache/ratis/pull/811#discussion_r1080912913
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -685,6 +687,22 @@ private synchronized void sendStartLeaderElectionToHigherPriorityPeer(RaftPeerId
});
}
+ boolean sendStartLeaderElection(FollowerInfo followerInfo) {
+ final RaftPeerId followerId = followerInfo.getPeer().getId();
+ final TermIndex leaderLastEntry = server.getState().getLastEntry();
+ if (leaderLastEntry == null) {
Review Comment:
I don't think `leaderLastEntry` could be null because line 386 here:
https://github.com/apache/ratis/blob/77a9949f98d6c80a8c1466887763d44fb64c9ccc/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java#L378-L390
However, it's good to check just in case.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -685,6 +687,22 @@ private synchronized void sendStartLeaderElectionToHigherPriorityPeer(RaftPeerId
});
}
+ boolean sendStartLeaderElection(FollowerInfo followerInfo) {
+ final RaftPeerId followerId = followerInfo.getPeer().getId();
+ final TermIndex leaderLastEntry = server.getState().getLastEntry();
+ if (leaderLastEntry == null) {
+ sendStartLeaderElection(followerId, null);
+ return true;
+ }
+
+ final long followerMatchIndex = followerInfo.getMatchIndex();
+ if (followerMatchIndex >= leaderLastEntry.getIndex()) {
Review Comment:
Same, I don't think `followerMatchIndex` can be greater than `leaderLastEntry.getIndex()`.
But it's okay to check just in case.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #811: RATIS-1772. Refactor the startLeaderElection code in LeaderStateImpl.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #811:
URL: https://github.com/apache/ratis/pull/811#discussion_r1080898247
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -685,6 +685,40 @@ private synchronized void sendStartLeaderElectionToHigherPriorityPeer(RaftPeerId
});
}
+ boolean sendStartLeaderElection(FollowerInfo followerInfo, int leaderPriority) {
Review Comment:
> ... find the peer with max priority first ...
After the change above, we no longer need to pass `leaderPriority` here.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #811: RATIS-1772. Refactor the startLeaderElection code in LeaderStateImpl.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #811:
URL: https://github.com/apache/ratis/pull/811#discussion_r1080961037
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -685,6 +687,22 @@ private synchronized void sendStartLeaderElectionToHigherPriorityPeer(RaftPeerId
});
}
+ boolean sendStartLeaderElection(FollowerInfo followerInfo) {
+ final RaftPeerId followerId = followerInfo.getPeer().getId();
+ final TermIndex leaderLastEntry = server.getState().getLastEntry();
+ if (leaderLastEntry == null) {
+ sendStartLeaderElection(followerId, null);
+ return true;
+ }
+
+ final long followerMatchIndex = followerInfo.getMatchIndex();
+ if (followerMatchIndex >= leaderLastEntry.getIndex()) {
Review Comment:
We do not hold a lock so that the `matchIndex` could (theoretically) be updated after `getLastEntry()`. I believe it won't happen in the current code though.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo merged pull request #811: RATIS-1772. Refactor the startLeaderElection code in LeaderStateImpl.
Posted by GitBox <gi...@apache.org>.
szetszwo merged PR #811:
URL: https://github.com/apache/ratis/pull/811
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen commented on a diff in pull request #811: RATIS-1772. Refactor the startLeaderElection code in LeaderStateImpl.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #811:
URL: https://github.com/apache/ratis/pull/811#discussion_r1080883413
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -685,6 +685,40 @@ private synchronized void sendStartLeaderElectionToHigherPriorityPeer(RaftPeerId
});
}
+ boolean sendStartLeaderElection(FollowerInfo followerInfo, int leaderPriority) {
Review Comment:
> and then pass null for `leaderPriority`.
`leaderPriority` is int, we cannot pass null to it.
And I think we should also check priority in TransferLeadership, otherwise `checkPeersForYieldingLeader()` will transfer it back very soon.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #811: RATIS-1772. Refactor the startLeaderElection code in LeaderStateImpl.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #811:
URL: https://github.com/apache/ratis/pull/811#discussion_r1080952565
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -685,6 +687,22 @@ private synchronized void sendStartLeaderElectionToHigherPriorityPeer(RaftPeerId
});
}
+ boolean sendStartLeaderElection(FollowerInfo followerInfo) {
+ final RaftPeerId followerId = followerInfo.getPeer().getId();
+ final TermIndex leaderLastEntry = server.getState().getLastEntry();
+ if (leaderLastEntry == null) {
Review Comment:
It (theoretically) could happen after created a snapshot and purged the log. However, the default values of the conf won't allow it.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #811: RATIS-1772. Refactor the startLeaderElection code in LeaderStateImpl.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #811:
URL: https://github.com/apache/ratis/pull/811#issuecomment-1397074810
@SzyWilliam , thanks a lot for reviewing this!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen commented on a diff in pull request #811: RATIS-1772. Refactor the startLeaderElection code in LeaderStateImpl.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #811:
URL: https://github.com/apache/ratis/pull/811#discussion_r1080880732
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -685,6 +685,40 @@ private synchronized void sendStartLeaderElectionToHigherPriorityPeer(RaftPeerId
});
}
+ boolean sendStartLeaderElection(FollowerInfo followerInfo, int leaderPriority) {
+ final RaftConfigurationImpl conf = server.getRaftConf();
+ final RaftPeerId followerId = followerInfo.getPeer().getId();
+ final RaftPeer follower = conf.getPeer(followerId);
+ if (follower == null) {
+ if (conf.getPeer(followerId, RaftPeerRole.LISTENER) == null) {
+ LOG.error("{} the follower {} is not in the conf {}", this, followerId, conf);
+ }
+ return false;
+ }
+ if (follower.getPriority() <= leaderPriority) {
+ return false;
Review Comment:
Please add a log before this line.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -685,6 +685,40 @@ private synchronized void sendStartLeaderElectionToHigherPriorityPeer(RaftPeerId
});
}
+ boolean sendStartLeaderElection(FollowerInfo followerInfo, int leaderPriority) {
+ final RaftConfigurationImpl conf = server.getRaftConf();
+ final RaftPeerId followerId = followerInfo.getPeer().getId();
+ final RaftPeer follower = conf.getPeer(followerId);
+ if (follower == null) {
+ if (conf.getPeer(followerId, RaftPeerRole.LISTENER) == null) {
+ LOG.error("{} the follower {} is not in the conf {}", this, followerId, conf);
+ }
+ return false;
Review Comment:
Please add a log before this line.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen commented on a diff in pull request #811: RATIS-1772. Refactor the startLeaderElection code in LeaderStateImpl.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #811:
URL: https://github.com/apache/ratis/pull/811#discussion_r1080855106
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -1024,49 +1057,15 @@ private long[] getSorted(List<FollowerInfo> followerInfos, boolean includeSelf,
return indices;
}
- private void yieldLeaderToHigherPriorityPeer() {
- if (!server.getInfo().isLeader()) {
- return;
- }
-
+ private void checkPeersForYieldingLeader() {
final RaftConfigurationImpl conf = server.getRaftConf();
final RaftPeer leader = conf.getPeer(server.getId());
if (leader == null) {
LOG.error("{} the leader {} is not in the conf {}", this, server.getId(), conf);
return;
}
- int leaderPriority = leader.getPriority();
-
for (LogAppender logAppender : senders.getSenders()) {
- final FollowerInfo followerInfo = logAppender.getFollower();
- final RaftPeerId followerID = followerInfo.getPeer().getId();
- final RaftPeer follower = conf.getPeer(followerID);
- if (follower == null) {
- if (conf.getPeer(followerID, RaftPeerRole.LISTENER) == null) {
- LOG.error("{} the follower {} is not in the conf {}", this, followerID, conf);
- }
- continue;
- }
- final int followerPriority = follower.getPriority();
- if (followerPriority <= leaderPriority) {
- continue;
- }
- final TermIndex leaderLastEntry = server.getState().getLastEntry();
- if (leaderLastEntry == null) {
- LOG.info("{} send StartLeaderElectionRequest to follower:{} on term:{} because follower's priority:{} " +
- "is higher than leader's:{} and leader's lastEntry is null",
- this, followerID, currentTerm, followerPriority, leaderPriority);
-
- sendStartLeaderElectionToHigherPriorityPeer(followerID, null);
- return;
- }
-
- if (followerInfo.getMatchIndex() >= leaderLastEntry.getIndex()) {
- LOG.info("{} send StartLeaderElectionRequest to follower:{} on term:{} because follower's priority:{} " +
- "is higher than leader's:{} and follower's lastEntry index:{} catch up with leader's:{}",
- this, followerID, currentTerm, followerPriority, leaderPriority, followerInfo.getMatchIndex(),
- leaderLastEntry.getIndex());
- sendStartLeaderElectionToHigherPriorityPeer(followerID, leaderLastEntry);
+ if (sendStartLeaderElection(logAppender.getFollower(), leader.getPriority())) {
Review Comment:
How about find the peer with max priority first, and then send StartLeaderElection.
Current code will find the first peer whose priority is higher than the leader, which may leads to unnecessary transfers.
For example:
| Node | Priority |
| --- | --- |
| Leader | 0 |
| Follower 1 | 1 |
| Follower 2 | 2 |
Leader should skip follower 1, and send StartLeaderElection to follower 2.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -685,6 +685,40 @@ private synchronized void sendStartLeaderElectionToHigherPriorityPeer(RaftPeerId
});
}
+ boolean sendStartLeaderElection(FollowerInfo followerInfo, int leaderPriority) {
Review Comment:
This abstraction is quite hard to be reused in TransferLeadership.
Because TransferLeadershipRequest contains `RaftPeerId transferee` instead of `FollowerInfo`. And `leaderPriority` sounds like an internal state of `LeaderStateImpl`.
Also, I think it's probably better to use another name instead of overload `sendStartLeaderElection`, because it's doing something else. And it may not send StartLeaderElection if conditions are not met.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -720,9 +754,8 @@ public void run() {
event.execute();
} else if (inStagingState()) {
checkStaging();
- } else {
- yieldLeaderToHigherPriorityPeer();
- checkLeadership();
+ } else if (checkLeadership()) {
+ checkPeersForYieldingLeader();
Review Comment:
Good job.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] kaijchen commented on a diff in pull request #811: RATIS-1772. Refactor the startLeaderElection code in LeaderStateImpl.
Posted by GitBox <gi...@apache.org>.
kaijchen commented on code in PR #811:
URL: https://github.com/apache/ratis/pull/811#discussion_r1080953934
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -685,6 +687,22 @@ private synchronized void sendStartLeaderElectionToHigherPriorityPeer(RaftPeerId
});
}
+ boolean sendStartLeaderElection(FollowerInfo followerInfo) {
+ final RaftPeerId followerId = followerInfo.getPeer().getId();
+ final TermIndex leaderLastEntry = server.getState().getLastEntry();
+ if (leaderLastEntry == null) {
Review Comment:
I see. Thanks for explaining.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #811: RATIS-1772. Refactor the startLeaderElection code in LeaderStateImpl.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #811:
URL: https://github.com/apache/ratis/pull/811#issuecomment-1396630755
@kaijchen , thanks a lot for the review and all the ideas!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org