szetszwo commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1139864198
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/TransferLeadership.java:
##########
@@ -151,29 +150,29 @@ private String tryTransferLeadership(LeaderStateImpl
leaderState, Context contex
return "LogAppender for transferee " + transferee + " is null";
}
final FollowerInfo follower = appender.getFollower();
- final String error = leaderState.sendStartLeaderElection(follower,
context.getLeaderLastEntry());
- if (error == null) {
- LOG.info("{}: sent StartLeaderElection to transferee {} immediately as
it already has up-to-date log",
+ if (follower == null) {
+ return "Follower for LogAppender" + appender + " is null";
Review Comment:
`appender.getFollower()` never returns null.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -663,7 +663,7 @@ CompletableFuture<RaftClientReply>
submitStepDownRequestAsync(TransferLeadership
return pendingStepDown.submitAsync(request);
}
- private synchronized String sendStartLeaderElection(RaftPeerId follower,
TermIndex lastEntry) {
+ synchronized String sendStartLeaderElection(RaftPeerId follower, TermIndex
lastEntry) {
Review Comment:
`synchronized` is incorrect since it does not change anything in
`LeaderStateImpl`.
Actually, this method has nothing to do with `LeaderStateImpl`. It should
be moved to `RaftServerImpl` since it use many methods there.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/TransferLeadership.java:
##########
@@ -151,29 +150,29 @@ private String tryTransferLeadership(LeaderStateImpl
leaderState, Context contex
return "LogAppender for transferee " + transferee + " is null";
}
final FollowerInfo follower = appender.getFollower();
- final String error = leaderState.sendStartLeaderElection(follower,
context.getLeaderLastEntry());
- if (error == null) {
- LOG.info("{}: sent StartLeaderElection to transferee {} immediately as
it already has up-to-date log",
+ if (follower == null) {
+ return "Follower for LogAppender" + appender + " is null";
+ }
+ if (LeaderStateImpl.isFollowerUpToDate(follower,
context.getLeaderLastEntry())) {
+ LOG.info("{}: sending StartLeaderElection to transferee {} immediately
as it already has up-to-date log",
+ server.getMemberId(), transferee);
Review Comment:
Let's move the LOG after `tryTransferLeadership(..)` returns. Then, we can
check the return value before log.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/TransferLeadership.java:
##########
@@ -134,12 +134,11 @@ boolean isSteppingDown() {
void onFollowerAppendEntriesReply(LeaderStateImpl leaderState, FollowerInfo
follower) {
// If the transferee has just append some entries and becomes up-to-date,
// send StartLeaderElection to it
- if (getTransferee().filter(t -> t.equals(follower.getId())).isPresent()) {
- final String error = leaderState.sendStartLeaderElection(follower,
leaderState.getLastEntry());
- if (error == null) {
- LOG.info("{}: sent StartLeaderElection to transferee {} after received
AppendEntriesResponse",
- server.getMemberId(), follower.getId());
- }
+ if (getTransferee().filter(t -> t.equals(follower.getId())).isPresent()
+ && LeaderStateImpl.isFollowerUpToDate(follower,
leaderState.getLastEntry())) {
+ LOG.info("{}: sending StartLeaderElection to transferee {} after
received AppendEntriesResponse",
+ server.getMemberId(), follower.getId());
+ leaderState.sendStartLeaderElection(follower.getId(),
leaderState.getLastEntry());
Review Comment:
- It should reuse `leaderState.getLastEntry()`.
- Check return value before LOG
```java
if (!getTransferee().filter(t ->
t.equals(follower.getId())).isPresent()) {
return;
}
final TermIndex lastEntry = leaderState.getLastEntry();
if (LeaderStateImpl.isFollowerUpToDate(follower, lastEntry)) {
final String error = sendStartLeaderElection(follower.getId(),
lastEntry);
if (error != null) {
LOG.warn("{}: Failed to sendStartLeaderElection to transferee {}",
server.getMemberId(), follower.getId());
} else {
LOG.info("{}: sent StartLeaderElection to transferee {} after
received AppendEntriesResponse",
server.getMemberId(), follower.getId());
}
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
