RATIS-1558. Support Listener in RaftServerImpl

[codings-dan]

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

[codings-dan]

@szetszwo Can you help review this patch, thanks!

[szetszwo]

@codings-dan , thanks a lot for working on this. It looks good in general. Some comments inlined.

[szetszwo]

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);
    }

[codings-dan]

done

[szetszwo]

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);

[codings-dan]

Thanks for pointing out this problem, in theory, old will not be a Listener, I have fixed it

[szetszwo]

Typo?

[codings-dan]

Sorry for causing this typo , i have fixed it

[szetszwo]

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();

[codings-dan]

done

[szetszwo]

Use shouldRun() from above.

[codings-dan]

done

[codings-dan]

@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.

[szetszwo]

@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.

[szetszwo]

+1 the change looks good.

[codings-dan]

@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!

[codings-dan]

• A Listener will reject requestVote. (this seems currently missing.)

@szetszwo This part doesn't seem to do anything since we don't send a requestVote request to the listener, see

ratis/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java

Line 279 in 117b333

final Collection<RaftPeer> others = conf.getOtherPeers(server.getId());

Thanks again for the guidance. I add unit test related to leaderElection, see #737
Combined with RATIS-1640 all unit test tasks have been completed.

[szetszwo]

@codings-dan , thanks a lot for working on the listener feature!

... since we don't send a requestVote request to the listener, ...

We should not depend on that. A patient problem is that a server may be misconfigured or do not have the up-to-date configuration. Then, it may send a requestVote to listeners, which it thinks they are voting members. If the listeners replied yes, the server will become a leader.