You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "kaijchen (via GitHub)" <gi...@apache.org> on 2023/03/07 03:06:52 UTC
[GitHub] [ratis] kaijchen opened a new pull request, #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
kaijchen opened a new pull request, #845:
URL: https://github.com/apache/ratis/pull/845
## What changes were proposed in this pull request?
Yield leader to higher priority peer by TransferLeadership.
To avoid blocking client requests for a long time,
it will only start TransferLeadership when the higher priority peer is up-to-date.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1770
## How was this patch tested?
Covered by `LeaderElectionTests#testYieldLeaderToHigherPriority()`.
--
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 pull request #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #845:
URL: https://github.com/apache/ratis/pull/845#issuecomment-1475162678
@szetszwo Thanks a lot for your review and suggestions.
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1139934406
##########
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:
Updated, I have swapped the two branches when checking `error`.
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1139934406
##########
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:
Updated.
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1140253659
##########
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:
> How about adding a Result class? Everything becomes clear and easy to understand.
OK, let me rebase new changes to https://issues.apache.org/jira/secure/attachment/13056384/845_review2.patch
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1139931807
##########
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:
OK, but tryTransferLeadership returns `null` doesn't mean we have sent StartLeaderElection immediately:
https://github.com/apache/ratis/pull/845/files/b19cccebaf2316fa9d16de21b77b4439f8514d33..d5d6b612c2015ade11e642041c1bdab77728f09a
--
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 pull request #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #845:
URL: https://github.com/apache/ratis/pull/845#issuecomment-1474170403
Rebased on https://issues.apache.org/jira/secure/attachment/13056384/845_review2.patch with fixes and improvements.
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1139879880
##########
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:
Just found that `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.
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1139879880
##########
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:
Just found that `synchronized` is incorrect since it does not change anything in `LeaderStateImpl`.
Actually, this method has nothing to do with `LeaderStateImpl`. Let move it to `TransferLeadership` since it is used there.
--
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 pull request #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #845:
URL: https://github.com/apache/ratis/pull/845#issuecomment-1473443285
Besides, since there are many refactors included in this PR, shall we change the title of this Jira?
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1138017225
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/TransferLeadership.java:
##########
@@ -93,49 +132,76 @@ boolean isSteppingDown() {
}
void onFollowerAppendEntriesReply(LeaderStateImpl leaderState, FollowerInfo follower) {
- final Optional<RaftPeerId> transferee = getTransferee();
// If the transferee has just append some entries and becomes up-to-date,
// send StartLeaderElection to it
- if (transferee.filter(t -> t.equals(follower.getId())).isPresent()
- && leaderState.sendStartLeaderElection(follower)) {
- LOG.info("{}: sent StartLeaderElection to transferee {} after received AppendEntriesResponse",
- server.getMemberId(), transferee.get());
+ 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());
+ }
}
}
- private void tryTransferLeadership(LeaderStateImpl leaderState, RaftPeerId transferee) {
+ private String tryTransferLeadership(LeaderStateImpl leaderState, Context context) {
+ final RaftPeerId transferee = context.getTransfereeId();
LOG.info("{}: start transferring leadership to {}", server.getMemberId(), transferee);
- final LogAppender appender = leaderState.getLogAppender(transferee).orElse(null);
-
+ final LogAppender appender = context.getTransfereeLogAppender();
if (appender == null) {
- LOG.error("{}: cannot find LogAppender for transferee {}", server.getMemberId(), transferee);
- return;
+ return "LogAppender for transferee " + transferee + " is null";
}
final FollowerInfo follower = appender.getFollower();
- if (leaderState.sendStartLeaderElection(follower)) {
+ 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",
server.getMemberId(), transferee);
- } else {
- LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date",
- server.getMemberId(), transferee);
+ } else if (error.contains("not up-to-date")) {
+ LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date: {}",
+ server.getMemberId(), transferee, error);
appender.notifyLogAppender();
+ return null;
}
+ return error;
+ }
+
+ TransferLeadershipRequest newRequest(RaftPeerId transfereeId) {
+ // TransferLeadership will block client request, so we don't want wait too long.
+ // If everything goes well, transferee should be elected within an election timeout.
+ final long timeout = server.getRandomElectionTimeout().toLong(TimeUnit.MILLISECONDS);
+ return new TransferLeadershipRequest(ClientId.emptyClientId(),
+ server.getId(), server.getMemberId().getGroupId(), 0, transfereeId, timeout);
+ }
+
+ void start(LeaderStateImpl leaderState, LogAppender transferee, TermIndex leaderLastEntry) {
+ start(leaderState, new Context(newRequest(transferee.getFollowerId()), () -> leaderLastEntry, () -> transferee));
}
CompletableFuture<RaftClientReply> start(LeaderStateImpl leaderState, TransferLeadershipRequest request) {
+ final Context context = new Context(request,
+ JavaUtils.memoize(leaderState::getLastEntry),
+ JavaUtils.memoize(() -> leaderState.getLogAppender(request.getNewLeader()).orElse(null)));
+ return start(leaderState, context);
+ }
+
+ private CompletableFuture<RaftClientReply> start(LeaderStateImpl leaderState, Context context) {
+ final TransferLeadershipRequest request = context.getRequest();
final MemoizedSupplier<PendingRequest> supplier = JavaUtils.memoize(() -> new PendingRequest(request));
final PendingRequest previous = pending.getAndUpdate(f -> f != null? f: supplier.get());
if (previous != null) {
- return createReplyFutureFromPreviousRequest(request, previous);
+ return request != null? createReplyFutureFromPreviousRequest(request, previous): null;
Review Comment:
Sorry, this needs to be reverted since `request` won't be null after we added `Context`.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/TransferLeadership.java:
##########
@@ -93,49 +132,76 @@ boolean isSteppingDown() {
}
void onFollowerAppendEntriesReply(LeaderStateImpl leaderState, FollowerInfo follower) {
- final Optional<RaftPeerId> transferee = getTransferee();
// If the transferee has just append some entries and becomes up-to-date,
// send StartLeaderElection to it
- if (transferee.filter(t -> t.equals(follower.getId())).isPresent()
- && leaderState.sendStartLeaderElection(follower)) {
- LOG.info("{}: sent StartLeaderElection to transferee {} after received AppendEntriesResponse",
- server.getMemberId(), transferee.get());
+ 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());
+ }
}
}
- private void tryTransferLeadership(LeaderStateImpl leaderState, RaftPeerId transferee) {
+ private String tryTransferLeadership(LeaderStateImpl leaderState, Context context) {
+ final RaftPeerId transferee = context.getTransfereeId();
LOG.info("{}: start transferring leadership to {}", server.getMemberId(), transferee);
- final LogAppender appender = leaderState.getLogAppender(transferee).orElse(null);
-
+ final LogAppender appender = context.getTransfereeLogAppender();
if (appender == null) {
- LOG.error("{}: cannot find LogAppender for transferee {}", server.getMemberId(), transferee);
- return;
+ return "LogAppender for transferee " + transferee + " is null";
}
final FollowerInfo follower = appender.getFollower();
- if (leaderState.sendStartLeaderElection(follower)) {
+ 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",
server.getMemberId(), transferee);
- } else {
- LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date",
- server.getMemberId(), transferee);
+ } else if (error.contains("not up-to-date")) {
+ LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date: {}",
+ server.getMemberId(), transferee, error);
appender.notifyLogAppender();
+ return null;
}
+ return error;
+ }
+
+ TransferLeadershipRequest newRequest(RaftPeerId transfereeId) {
Review Comment:
Now `newRequest` is used once. Let's move the code to the `start(..)` method below.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/TransferLeadership.java:
##########
@@ -93,49 +132,76 @@ boolean isSteppingDown() {
}
void onFollowerAppendEntriesReply(LeaderStateImpl leaderState, FollowerInfo follower) {
- final Optional<RaftPeerId> transferee = getTransferee();
// If the transferee has just append some entries and becomes up-to-date,
// send StartLeaderElection to it
- if (transferee.filter(t -> t.equals(follower.getId())).isPresent()
- && leaderState.sendStartLeaderElection(follower)) {
- LOG.info("{}: sent StartLeaderElection to transferee {} after received AppendEntriesResponse",
- server.getMemberId(), transferee.get());
+ 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());
+ }
}
}
- private void tryTransferLeadership(LeaderStateImpl leaderState, RaftPeerId transferee) {
+ private String tryTransferLeadership(LeaderStateImpl leaderState, Context context) {
+ final RaftPeerId transferee = context.getTransfereeId();
LOG.info("{}: start transferring leadership to {}", server.getMemberId(), transferee);
- final LogAppender appender = leaderState.getLogAppender(transferee).orElse(null);
-
+ final LogAppender appender = context.getTransfereeLogAppender();
if (appender == null) {
- LOG.error("{}: cannot find LogAppender for transferee {}", server.getMemberId(), transferee);
- return;
+ return "LogAppender for transferee " + transferee + " is null";
}
final FollowerInfo follower = appender.getFollower();
- if (leaderState.sendStartLeaderElection(follower)) {
+ 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",
server.getMemberId(), transferee);
- } else {
- LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date",
- server.getMemberId(), transferee);
+ } else if (error.contains("not up-to-date")) {
Review Comment:
Let's add a `Result` class to avoid depending on error message.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/TransferLeadership.java:
##########
@@ -93,49 +132,76 @@ boolean isSteppingDown() {
}
void onFollowerAppendEntriesReply(LeaderStateImpl leaderState, FollowerInfo follower) {
- final Optional<RaftPeerId> transferee = getTransferee();
// If the transferee has just append some entries and becomes up-to-date,
// send StartLeaderElection to it
- if (transferee.filter(t -> t.equals(follower.getId())).isPresent()
- && leaderState.sendStartLeaderElection(follower)) {
- LOG.info("{}: sent StartLeaderElection to transferee {} after received AppendEntriesResponse",
- server.getMemberId(), transferee.get());
+ 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());
+ }
}
}
- private void tryTransferLeadership(LeaderStateImpl leaderState, RaftPeerId transferee) {
+ private String tryTransferLeadership(LeaderStateImpl leaderState, Context context) {
+ final RaftPeerId transferee = context.getTransfereeId();
LOG.info("{}: start transferring leadership to {}", server.getMemberId(), transferee);
- final LogAppender appender = leaderState.getLogAppender(transferee).orElse(null);
-
+ final LogAppender appender = context.getTransfereeLogAppender();
if (appender == null) {
- LOG.error("{}: cannot find LogAppender for transferee {}", server.getMemberId(), transferee);
- return;
+ return "LogAppender for transferee " + transferee + " is null";
}
final FollowerInfo follower = appender.getFollower();
- if (leaderState.sendStartLeaderElection(follower)) {
+ 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",
server.getMemberId(), transferee);
- } else {
- LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date",
- server.getMemberId(), transferee);
+ } else if (error.contains("not up-to-date")) {
+ LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date: {}",
+ server.getMemberId(), transferee, error);
appender.notifyLogAppender();
+ return null;
}
+ return error;
+ }
+
+ TransferLeadershipRequest newRequest(RaftPeerId transfereeId) {
+ // TransferLeadership will block client request, so we don't want wait too long.
+ // If everything goes well, transferee should be elected within an election timeout.
+ final long timeout = server.getRandomElectionTimeout().toLong(TimeUnit.MILLISECONDS);
Review Comment:
It should use `server.properties().minRpcTimeoutMs()` (or max). Random does not sound correct.
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1138085595
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/TransferLeadership.java:
##########
@@ -93,49 +132,76 @@ boolean isSteppingDown() {
}
void onFollowerAppendEntriesReply(LeaderStateImpl leaderState, FollowerInfo follower) {
- final Optional<RaftPeerId> transferee = getTransferee();
// If the transferee has just append some entries and becomes up-to-date,
// send StartLeaderElection to it
- if (transferee.filter(t -> t.equals(follower.getId())).isPresent()
- && leaderState.sendStartLeaderElection(follower)) {
- LOG.info("{}: sent StartLeaderElection to transferee {} after received AppendEntriesResponse",
- server.getMemberId(), transferee.get());
+ 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());
+ }
}
}
- private void tryTransferLeadership(LeaderStateImpl leaderState, RaftPeerId transferee) {
+ private String tryTransferLeadership(LeaderStateImpl leaderState, Context context) {
+ final RaftPeerId transferee = context.getTransfereeId();
LOG.info("{}: start transferring leadership to {}", server.getMemberId(), transferee);
- final LogAppender appender = leaderState.getLogAppender(transferee).orElse(null);
-
+ final LogAppender appender = context.getTransfereeLogAppender();
if (appender == null) {
- LOG.error("{}: cannot find LogAppender for transferee {}", server.getMemberId(), transferee);
- return;
+ return "LogAppender for transferee " + transferee + " is null";
}
final FollowerInfo follower = appender.getFollower();
- if (leaderState.sendStartLeaderElection(follower)) {
+ 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",
server.getMemberId(), transferee);
- } else {
- LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date",
- server.getMemberId(), transferee);
+ } else if (error.contains("not up-to-date")) {
+ LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date: {}",
+ server.getMemberId(), transferee, error);
appender.notifyLogAppender();
+ return null;
}
+ return error;
+ }
+
+ TransferLeadershipRequest newRequest(RaftPeerId transfereeId) {
+ // TransferLeadership will block client request, so we don't want wait too long.
+ // If everything goes well, transferee should be elected within an election timeout.
+ final long timeout = server.getRandomElectionTimeout().toLong(TimeUnit.MILLISECONDS);
Review Comment:
Yes, they are for server rpc, i.e. (appendEntries).
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1138084517
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/TransferLeadership.java:
##########
@@ -93,49 +132,76 @@ boolean isSteppingDown() {
}
void onFollowerAppendEntriesReply(LeaderStateImpl leaderState, FollowerInfo follower) {
- final Optional<RaftPeerId> transferee = getTransferee();
// If the transferee has just append some entries and becomes up-to-date,
// send StartLeaderElection to it
- if (transferee.filter(t -> t.equals(follower.getId())).isPresent()
- && leaderState.sendStartLeaderElection(follower)) {
- LOG.info("{}: sent StartLeaderElection to transferee {} after received AppendEntriesResponse",
- server.getMemberId(), transferee.get());
+ 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());
+ }
}
}
- private void tryTransferLeadership(LeaderStateImpl leaderState, RaftPeerId transferee) {
+ private String tryTransferLeadership(LeaderStateImpl leaderState, Context context) {
+ final RaftPeerId transferee = context.getTransfereeId();
LOG.info("{}: start transferring leadership to {}", server.getMemberId(), transferee);
- final LogAppender appender = leaderState.getLogAppender(transferee).orElse(null);
-
+ final LogAppender appender = context.getTransfereeLogAppender();
if (appender == null) {
- LOG.error("{}: cannot find LogAppender for transferee {}", server.getMemberId(), transferee);
- return;
+ return "LogAppender for transferee " + transferee + " is null";
}
final FollowerInfo follower = appender.getFollower();
- if (leaderState.sendStartLeaderElection(follower)) {
+ 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",
server.getMemberId(), transferee);
- } else {
- LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date",
- server.getMemberId(), transferee);
+ } else if (error.contains("not up-to-date")) {
Review Comment:
There is another approach, I removed the overloaded `sendStartElection` (the non-synchronized one).
The caller should check `isFollowerUpToDate` before calling `sendStartElection`.
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1138123538
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/TransferLeadership.java:
##########
@@ -93,49 +132,76 @@ boolean isSteppingDown() {
}
void onFollowerAppendEntriesReply(LeaderStateImpl leaderState, FollowerInfo follower) {
- final Optional<RaftPeerId> transferee = getTransferee();
// If the transferee has just append some entries and becomes up-to-date,
// send StartLeaderElection to it
- if (transferee.filter(t -> t.equals(follower.getId())).isPresent()
- && leaderState.sendStartLeaderElection(follower)) {
- LOG.info("{}: sent StartLeaderElection to transferee {} after received AppendEntriesResponse",
- server.getMemberId(), transferee.get());
+ 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());
+ }
}
}
- private void tryTransferLeadership(LeaderStateImpl leaderState, RaftPeerId transferee) {
+ private String tryTransferLeadership(LeaderStateImpl leaderState, Context context) {
+ final RaftPeerId transferee = context.getTransfereeId();
LOG.info("{}: start transferring leadership to {}", server.getMemberId(), transferee);
- final LogAppender appender = leaderState.getLogAppender(transferee).orElse(null);
-
+ final LogAppender appender = context.getTransfereeLogAppender();
if (appender == null) {
- LOG.error("{}: cannot find LogAppender for transferee {}", server.getMemberId(), transferee);
- return;
+ return "LogAppender for transferee " + transferee + " is null";
}
final FollowerInfo follower = appender.getFollower();
- if (leaderState.sendStartLeaderElection(follower)) {
+ 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",
server.getMemberId(), transferee);
- } else {
- LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date",
- server.getMemberId(), transferee);
+ } else if (error.contains("not up-to-date")) {
+ LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date: {}",
+ server.getMemberId(), transferee, error);
appender.notifyLogAppender();
+ return null;
}
+ return error;
+ }
+
+ TransferLeadershipRequest newRequest(RaftPeerId transfereeId) {
+ // TransferLeadership will block client request, so we don't want wait too long.
+ // If everything goes well, transferee should be elected within an election timeout.
+ final long timeout = server.getRandomElectionTimeout().toLong(TimeUnit.MILLISECONDS);
Review Comment:
Sorry, I was not clear -- It is the timeout to for sending out heartbeat appendEntries. (Not the timeout to wait for an appendEntries reply)
--
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 pull request #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #845:
URL: https://github.com/apache/ratis/pull/845#issuecomment-1473399123
Thanks @szetszwo for the review and suggestion.
Changes since your last review: https://github.com/apache/ratis/pull/845/files/060df2c530dcb73344f18e4db45586861ba39d81..23810a1308085d3db12ec8d9006b4a434cf9c5a1
Changes on top of patch-3:
https://github.com/apache/ratis/pull/845/files/8e150d503eca5fc5681cf6cd367fdfe726860fe4..23810a1308085d3db12ec8d9006b4a434cf9c5a1
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1140226839
##########
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:
@kaijchen , you are right. How about adding a `Result` class? Everything becomes clear and easy to understand.
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
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: 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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1130224613
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -684,16 +685,16 @@ private synchronized void sendStartLeaderElection(RaftPeerId follower, TermIndex
});
}
+ boolean isFollowerUpToDate(FollowerInfo followerInfo) {
+ final TermIndex leaderLastEntry = server.getState().getLastEntry();
Review Comment:
It should not call getLastEntry() two times.
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1140975322
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/TransferLeadership.java:
##########
@@ -92,50 +188,135 @@ boolean isSteppingDown() {
return pending.get() != null;
}
+ static Result isFollowerUpToDate(FollowerInfo follower, TermIndex leaderLastEntry) {
+ if (follower == null) {
+ return Result.NULL_FOLLOWER;
+ } else if (leaderLastEntry != null) {
+ final long followerMatchIndex = follower.getMatchIndex();
+ if (followerMatchIndex < leaderLastEntry.getIndex()) {
+ return new Result(Result.Type.NOT_UP_TO_DATE, "followerMatchIndex = " + followerMatchIndex
+ + " < leaderLastEntry.getIndex() = " + leaderLastEntry.getIndex());
+ }
+ }
+ return Result.SUCCESS;
+ }
+
+ private Result sendStartLeaderElection(FollowerInfo follower, TermIndex lastEntry) {
+ final Result result = isFollowerUpToDate(follower, lastEntry);
+ if (result != Result.SUCCESS) {
+ return result;
+ }
+
+ final TermIndex currLastEntry = server.getState().getLastEntry();
+ if (ServerState.compareLog(currLastEntry, lastEntry) != 0) {
+ return new Result(Result.Type.LAST_ENTRY_CHANGED,
Review Comment:
I'm wondering why we should abort TransferLeadership when `LAST_ENTRY_CHANGED`.
The `StartLeaderElection` request shoud be idempotent (by including sender's term).
Then, as long as follower is up-to-date, it is OK to send the request.
i.e. Only check `isFollowerUpToDate(follower, currLastEntry)`.
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1139934683
##########
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:
Good catch. Thanks for the suggestion.
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1138087986
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/TransferLeadership.java:
##########
@@ -93,49 +132,76 @@ boolean isSteppingDown() {
}
void onFollowerAppendEntriesReply(LeaderStateImpl leaderState, FollowerInfo follower) {
- final Optional<RaftPeerId> transferee = getTransferee();
// If the transferee has just append some entries and becomes up-to-date,
// send StartLeaderElection to it
- if (transferee.filter(t -> t.equals(follower.getId())).isPresent()
- && leaderState.sendStartLeaderElection(follower)) {
- LOG.info("{}: sent StartLeaderElection to transferee {} after received AppendEntriesResponse",
- server.getMemberId(), transferee.get());
+ 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());
+ }
}
}
- private void tryTransferLeadership(LeaderStateImpl leaderState, RaftPeerId transferee) {
+ private String tryTransferLeadership(LeaderStateImpl leaderState, Context context) {
+ final RaftPeerId transferee = context.getTransfereeId();
LOG.info("{}: start transferring leadership to {}", server.getMemberId(), transferee);
- final LogAppender appender = leaderState.getLogAppender(transferee).orElse(null);
-
+ final LogAppender appender = context.getTransfereeLogAppender();
if (appender == null) {
- LOG.error("{}: cannot find LogAppender for transferee {}", server.getMemberId(), transferee);
- return;
+ return "LogAppender for transferee " + transferee + " is null";
}
final FollowerInfo follower = appender.getFollower();
- if (leaderState.sendStartLeaderElection(follower)) {
+ 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",
server.getMemberId(), transferee);
- } else {
- LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date",
- server.getMemberId(), transferee);
+ } else if (error.contains("not up-to-date")) {
+ LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date: {}",
+ server.getMemberId(), transferee, error);
appender.notifyLogAppender();
+ return null;
}
+ return error;
+ }
+
+ TransferLeadershipRequest newRequest(RaftPeerId transfereeId) {
+ // TransferLeadership will block client request, so we don't want wait too long.
+ // If everything goes well, transferee should be elected within an election timeout.
+ final long timeout = server.getRandomElectionTimeout().toLong(TimeUnit.MILLISECONDS);
Review Comment:
Does it mean, it is possible for a follower to start election as soon as an appendEntries times out?
Should we seperate these two timeouts (in the future)?
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #845:
URL: https://github.com/apache/ratis/pull/845#issuecomment-1464057234
@kaijchen , I reviewed this and the TransferLeadership code earlier and found some suggestions. Will post some comments 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] kaijchen commented on a diff in pull request #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1138035396
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/TransferLeadership.java:
##########
@@ -93,49 +132,76 @@ boolean isSteppingDown() {
}
void onFollowerAppendEntriesReply(LeaderStateImpl leaderState, FollowerInfo follower) {
- final Optional<RaftPeerId> transferee = getTransferee();
// If the transferee has just append some entries and becomes up-to-date,
// send StartLeaderElection to it
- if (transferee.filter(t -> t.equals(follower.getId())).isPresent()
- && leaderState.sendStartLeaderElection(follower)) {
- LOG.info("{}: sent StartLeaderElection to transferee {} after received AppendEntriesResponse",
- server.getMemberId(), transferee.get());
+ 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());
+ }
}
}
- private void tryTransferLeadership(LeaderStateImpl leaderState, RaftPeerId transferee) {
+ private String tryTransferLeadership(LeaderStateImpl leaderState, Context context) {
+ final RaftPeerId transferee = context.getTransfereeId();
LOG.info("{}: start transferring leadership to {}", server.getMemberId(), transferee);
- final LogAppender appender = leaderState.getLogAppender(transferee).orElse(null);
-
+ final LogAppender appender = context.getTransfereeLogAppender();
if (appender == null) {
- LOG.error("{}: cannot find LogAppender for transferee {}", server.getMemberId(), transferee);
- return;
+ return "LogAppender for transferee " + transferee + " is null";
}
final FollowerInfo follower = appender.getFollower();
- if (leaderState.sendStartLeaderElection(follower)) {
+ 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",
server.getMemberId(), transferee);
- } else {
- LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date",
- server.getMemberId(), transferee);
+ } else if (error.contains("not up-to-date")) {
+ LOG.info("{}: notifying LogAppender to send AppendEntries as transferee {} is not up-to-date: {}",
+ server.getMemberId(), transferee, error);
appender.notifyLogAppender();
+ return null;
}
+ return error;
+ }
+
+ TransferLeadershipRequest newRequest(RaftPeerId transfereeId) {
+ // TransferLeadership will block client request, so we don't want wait too long.
+ // If everything goes well, transferee should be elected within an election timeout.
+ final long timeout = server.getRandomElectionTimeout().toLong(TimeUnit.MILLISECONDS);
Review Comment:
I just looked at `getRandomElectionTimeout()`, and timeout in range `[minRpcTimeoutMs, maxRpcTimeoutMs]` is currently used for election timeout.
What does `minRpcTimeoutMs` and `maxRpcTimeoutMs` mean? Are they only used for election, or also for rpc timeout as the name suggests?
--
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 pull request #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #845:
URL: https://github.com/apache/ratis/pull/845#issuecomment-1469906749
@szetszwo based on your patch, I have added some fixes and improvements.
Should we change the default timeout of ratis shell in this PR also?
| TransferLeadership by | default timeout |
| --- | --- |
| client (ratis shell) | `3s` (hard-coded) |
| client (non ratis shell) | default request timeout |
| server (priority yield) | random election timeout |
--
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 #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #845:
URL: https://github.com/apache/ratis/pull/845#discussion_r1139932774
##########
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:
Removed, this was added in https://github.com/apache/ratis/pull/845/commits/060df2c530dcb73344f18e4db45586861ba39d81 to match the old logic 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 merged pull request #845: RATIS-1770. Yield leader to higher priority peer by TransferLeadership
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #845:
URL: https://github.com/apache/ratis/pull/845
--
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