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 2022/03/23 13:22:59 UTC
[GitHub] [ratis] codings-dan opened a new pull request #627: RATIS-1558. Support Listener in RaftServerImpl
codings-dan opened a new pull request #627:
URL: https://github.com/apache/ratis/pull/627
## What changes were proposed in this pull request?
subtask to support listener: Support Listener in RaftServerImpl
## What is the link to the Apache JIRA
https://issues.apache.org/jira/projects/RATIS/issues/RATIS-1558?filter=reportedbyme
## How was this patch tested?
UT
--
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 change in pull request #627: RATIS-1558. Support Listener in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #627:
URL: https://github.com/apache/ratis/pull/627#discussion_r834037907
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/FollowerState.java
##########
@@ -112,7 +112,7 @@ boolean lostMajorityHeartbeatsRecently() {
@Override
public void run() {
final TimeDuration sleepDeviationThreshold = server.getSleepDeviationThreshold();
- while (isRunning && server.getInfo().isFollower()) {
+ while (isRunning && (server.getInfo().isFollower() || server.getInfo().isListener())) {
Review comment:
Let's add a shouldRun() method.
```
@@ -109,10 +110,19 @@ class FollowerState extends Daemon {
return true;
}
+ private boolean shouldRun() {
+ final DivisionInfo info = server.getInfo();
+ final boolean run = isRunning && (info.isFollower() || info.isListener());
+ if (!run) {
+ LOG.info("{}: Stopping now (isRunning? {}, role = {})", this, isRunning, info.getCurrentRole());
+ }
+ return run;
+ }
+
@Override
public void run() {
final TimeDuration sleepDeviationThreshold = server.getSleepDeviationThreshold();
- while (isRunning && server.getInfo().isFollower()) {
+ while (shouldRun()) {
final TimeDuration electionTimeout = server.getRandomElectionTimeout();
try {
final TimeDuration extraSleep = electionTimeout.sleep();
```
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -1338,7 +1354,7 @@ private CommitInfoProto updateCommitInfoCache() {
}
followerState = updateLastRpcTime(FollowerState.UpdateType.APPEND_START);
- // Check that the append entries are not inconsistent. There are 3
+ // Check that the append ents are not inconsistent. There are 3
Review comment:
Typo?
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -335,11 +338,19 @@ static boolean registerMBean(
}
/**
- * The peer belongs to the current configuration, should start as a follower
+ * The peer belongs to the current configuration, should start as a follower or listener
*/
- private void startAsFollower() {
- setRole(RaftPeerRole.FOLLOWER, "startAsFollower");
- role.startFollowerState(this, "startAsFollower");
+ private void startAsPeer(RaftPeerRole roles) {
+ Object reason = "";
+ if (roles.equals(RaftPeerRole.FOLLOWER)) {
+ reason = "startAsFollower";
+ setRole(RaftPeerRole.FOLLOWER, reason);
+ } else if (roles.equals(RaftPeerRole.LISTENER)) {
Review comment:
Since RaftPeerRole is enum, use == . Also, let's add an else-clause to throw an exception.
```
if (newRole == RaftPeerRole.FOLLOWER) {
reason = "startAsFollower";
setRole(RaftPeerRole.FOLLOWER, reason);
} else if (newRole == RaftPeerRole.LISTENER) {
reason = "startAsListener";
setRole(RaftPeerRole.LISTENER, reason);
} else {
throw new IllegalArgumentException("Unexpected role " + newRole);
}
```
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -488,12 +499,17 @@ private synchronized boolean changeToFollower(long newTerm, boolean force, Objec
final boolean metadataUpdated = state.updateCurrentTerm(newTerm);
if (old != RaftPeerRole.FOLLOWER || force) {
- setRole(RaftPeerRole.FOLLOWER, reason);
+ if (old != RaftPeerRole.LISTENER) {
Review comment:
It is illegal to have old == LISTENER. Let's throw an exception in the beginning. Then we don't have to change other code in this method.
```
@@ -485,6 +498,9 @@ class RaftServerImpl implements RaftServer.Division,
*/
private synchronized boolean changeToFollower(long newTerm, boolean force, Object reason) {
final RaftPeerRole old = role.getCurrentRole();
+ if (old == RaftPeerRole.LISTENER) {
+ throw new IllegalStateException("Unexpected role " + old);
+ }
final boolean metadataUpdated = state.updateCurrentTerm(newTerm);
```
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/FollowerState.java
##########
@@ -122,14 +122,14 @@ public void run() {
continue;
}
- final boolean isFollower = server.getInfo().isFollower();
- if (!isRunning || !isFollower) {
- LOG.info("{}: Stopping now (isRunning? {}, isFollower? {})", this, isRunning, isFollower);
+ final boolean isPeer = (server.getInfo().isFollower() || server.getInfo().isListener());
+ if (!isRunning || !isPeer) {
+ LOG.info("{}: Stopping now (isRunning? {}, isPeer? {})", this, isRunning, isPeer);
Review comment:
Use shouldRun() from above.
--
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] codings-dan commented on a change in pull request #627: RATIS-1558. Support Listener in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
codings-dan commented on a change in pull request #627:
URL: https://github.com/apache/ratis/pull/627#discussion_r834111144
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -335,11 +338,19 @@ static boolean registerMBean(
}
/**
- * The peer belongs to the current configuration, should start as a follower
+ * The peer belongs to the current configuration, should start as a follower or listener
*/
- private void startAsFollower() {
- setRole(RaftPeerRole.FOLLOWER, "startAsFollower");
- role.startFollowerState(this, "startAsFollower");
+ private void startAsPeer(RaftPeerRole roles) {
+ Object reason = "";
+ if (roles.equals(RaftPeerRole.FOLLOWER)) {
+ reason = "startAsFollower";
+ setRole(RaftPeerRole.FOLLOWER, reason);
+ } else if (roles.equals(RaftPeerRole.LISTENER)) {
Review comment:
done
--
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] codings-dan commented on pull request #627: RATIS-1558. Support Listener in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
codings-dan commented on pull request #627:
URL: https://github.com/apache/ratis/pull/627#issuecomment-1077442892
@szetszwo Thanks for reviewing the code, I change the code according to the comment you left, PTAL again, thank you! BTW, in my opinion, apart from adding a admin command to change the listener to follower using the `setConfiguration` API , the development work related to the listener has been completed. If I missed any part of the task, Can you help point it out? I can follow your suggestion to continue the follow-up development work.
--
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] codings-dan commented on pull request #627: RATIS-1558. Support Listener in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
codings-dan commented on pull request #627:
URL: https://github.com/apache/ratis/pull/627#issuecomment-1077610791
> @codings-dan , thanks a lot for working on the Listener! We should add some tests to make sure the following.
>
> * A Listener won't start leader election.
> * A Listener will reject requestVote. (this seems currently missing.)
> * A Listener can receive log entires and snapshots.
> * The leader won't count Listeners for majority.
> * setConfiguration can add Listeners, remove Listeners and, change a voting member to Listener and vice versa.
will do, thanks for the guidance!
--
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] codings-dan commented on pull request #627: RATIS-1558. Support Listener in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
codings-dan commented on pull request #627:
URL: https://github.com/apache/ratis/pull/627#issuecomment-1076986194
@szetszwo Can you help review this patch, thanks!
--
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] codings-dan commented on a change in pull request #627: RATIS-1558. Support Listener in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
codings-dan commented on a change in pull request #627:
URL: https://github.com/apache/ratis/pull/627#discussion_r834114485
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -488,12 +499,17 @@ private synchronized boolean changeToFollower(long newTerm, boolean force, Objec
final boolean metadataUpdated = state.updateCurrentTerm(newTerm);
if (old != RaftPeerRole.FOLLOWER || force) {
- setRole(RaftPeerRole.FOLLOWER, reason);
+ if (old != RaftPeerRole.LISTENER) {
Review comment:
Thanks for pointing out this problem, in theory, old will not be a Listener, I have fixed 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] codings-dan commented on a change in pull request #627: RATIS-1558. Support Listener in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
codings-dan commented on a change in pull request #627:
URL: https://github.com/apache/ratis/pull/627#discussion_r834112553
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -1338,7 +1354,7 @@ private CommitInfoProto updateCommitInfoCache() {
}
followerState = updateLastRpcTime(FollowerState.UpdateType.APPEND_START);
- // Check that the append entries are not inconsistent. There are 3
+ // Check that the append ents are not inconsistent. There are 3
Review comment:
Sorry for causing this typo , i have fixed it
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/FollowerState.java
##########
@@ -112,7 +112,7 @@ boolean lostMajorityHeartbeatsRecently() {
@Override
public void run() {
final TimeDuration sleepDeviationThreshold = server.getSleepDeviationThreshold();
- while (isRunning && server.getInfo().isFollower()) {
+ while (isRunning && (server.getInfo().isFollower() || server.getInfo().isListener())) {
Review comment:
done
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/FollowerState.java
##########
@@ -122,14 +122,14 @@ public void run() {
continue;
}
- final boolean isFollower = server.getInfo().isFollower();
- if (!isRunning || !isFollower) {
- LOG.info("{}: Stopping now (isRunning? {}, isFollower? {})", this, isRunning, isFollower);
+ final boolean isPeer = (server.getInfo().isFollower() || server.getInfo().isListener());
+ if (!isRunning || !isPeer) {
+ LOG.info("{}: Stopping now (isRunning? {}, isPeer? {})", this, isRunning, isPeer);
Review comment:
done
--
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 #627: RATIS-1558. Support Listener in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #627:
URL: https://github.com/apache/ratis/pull/627
--
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 #627: RATIS-1558. Support Listener in RaftServerImpl
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #627:
URL: https://github.com/apache/ratis/pull/627#issuecomment-1077578507
@codings-dan , thanks a lot for working on the Listener! We should add some tests to make sure the following.
- A Listener won't start leader election.
- A Listener will reject requestVote. (this seems currently missing.)
- A Listener can receive log entires and snapshots.
- The leader won't count Listeners for majority.
- setConfiguration can add Listeners, remove Listeners and, change a voting member to Listener and vice versa.
--
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