RATIS-1594. Support ADD mode to SetConfiguration #658

codings-dan · 2022-06-20T12:55:57Z

see https://issues.apache.org/jira/browse/RATIS-1594

codings-dan · 2022-06-20T13:33:17Z

@szetszwo @SzyWilliam Can you help review this patch, thanks!

SzyWilliam

LGTM : )

szetszwo

@codings-dan , thanks a lot for working on this! Some comments inlined.

We should think about what should happen in ADD mode when a new peer already exists in the current peer list? Should the request fail? Cc @SzyWilliam .

szetszwo · 2022-06-20T19:40:02Z

ratis-client/src/main/java/org/apache/ratis/client/api/AdminApi.java

+  RaftClientReply setConfiguration(List<RaftPeer> serversInNewConf, SetConfigurationRequest.Mode mode)
+      throws IOException;
+
  /** Set the configuration request to the raft service. */
  RaftClientReply setConfiguration(List<RaftPeer> serversInNewConf, List<RaftPeer> listenersInNewConf)
      throws IOException;

+  RaftClientReply setConfiguration(List<RaftPeer> serversInNewConf, List<RaftPeer> listenersInNewConf,
+      SetConfigurationRequest.Mode mode) throws IOException;
+


With ADD and CAS modes, the combination of arguments becomes complicated. Let's add a SetConfigurationArguments class and a Builder.

Good catch, I have refactored this part of the interface related code, please take a look, thank you!

szetszwo · 2022-06-20T19:44:20Z

ratis-common/src/main/java/org/apache/ratis/protocol/SetConfigurationRequest.java

@@ -23,19 +23,26 @@
 import java.util.List;

 public class SetConfigurationRequest extends RaftClientRequest {
+
+  public enum Mode {
+    NORMAL,


Let's call it SET_UNCONDITIONALLY.

szetszwo · 2022-06-20T19:48:51Z

ratis-proto/src/main/proto/Raft.proto

@@ -409,6 +409,12 @@ message SetConfigurationRequestProto {
  RaftRpcRequestProto rpcRequest = 1;
  repeated RaftPeerProto peers = 2;
  repeated RaftPeerProto listeners = 3;
+  enum RaftConfigurationMode {


Similar to SetConfigurationRequest, let's simply call it "Mode". Also, please move the enum to the top.

szetszwo · 2022-06-20T20:05:57Z

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

+         List<RaftPeer> peers = Stream.of(peersInNewConf, new ArrayList<>(current.getAllPeers()))
+            .flatMap(List :: stream).distinct().collect(Collectors.toList());


What should happen in ADD mode when a new peer already exists in the current peer list? Should the request fail?

emm, I think if the new peer already exists in the current peer list, we could return success, I have change the related code, PTAL, thank you!

But the new peer may have a different priority. Should we allow it?

I think we only check peers that are not in the current list, other semantics do not change, what do you think?

When ADD an existing peer with a different priority, we may either

return failure and the configuration remains unchanged, or

return success and the configuration changes to the new server priority.

If we do the following, then the user request (priority change) is ignored silently. It sounds like a bug.

return success and the configuration remains unchanged.

The current code already implements the second point:

return success and the configuration changes to the new server priority.

Please take a look at RaftConfigurationImpl#hasNoChange

@codings-dan , But we use distinct() so that we are not sure whether the existing or the new server is used.

BTW, RaftConfigurationImpl#hasNoChange does not check listeners at all. It is a bug.

szetszwo · 2022-06-21T03:36:23Z

Below are some rough ideas. Forgot to post it previously.

diff --git a/ratis-client/src/main/java/org/apache/ratis/client/api/AdminApi.java b/ratis-client/src/main/java/org/apache/ratis/client/api/AdminApi.java
index c9c3f7b4..fc11c9b0 100644
--- a/ratis-client/src/main/java/org/apache/ratis/client/api/AdminApi.java
+++ b/ratis-client/src/main/java/org/apache/ratis/client/api/AdminApi.java
@@ -32,6 +32,22 @@ import java.util.List;
  * such as setting raft configuration and transferring leadership.
  */
 public interface AdminApi {
+  class SetConfigurationArguments {
+    // TODO add details
+
+    public static Builder newBuilder() {
+      return new Builder();
+    }
+
+    public static class Builder {
+      // TODO add details
+
+      public SetConfigurationArguments build() {
+        return null;
+      }
+    }
+  }
+
   /** The same as setConfiguration(serversInNewConf, Collections.emptyList()). */
   default RaftClientReply setConfiguration(List<RaftPeer> serversInNewConf) throws IOException {
     return setConfiguration(serversInNewConf, Collections.emptyList());
@@ -42,15 +58,15 @@ public interface AdminApi {
     return setConfiguration(Arrays.asList(serversInNewConf));
   }
 
-  RaftClientReply setConfiguration(List<RaftPeer> serversInNewConf, SetConfigurationRequest.Mode mode)
-      throws IOException;
-
   /** Set the configuration request to the raft service. */
-  RaftClientReply setConfiguration(List<RaftPeer> serversInNewConf, List<RaftPeer> listenersInNewConf)
-      throws IOException;
+  default RaftClientReply setConfiguration(List<RaftPeer> serversInNewConf, List<RaftPeer> listenersInNewConf)
+      throws IOException {
+    // TODO use Builder
+    return setConfiguration(SetConfigurationArguments.newBuilder().build());
+  }
 
-  RaftClientReply setConfiguration(List<RaftPeer> serversInNewConf, List<RaftPeer> listenersInNewConf,
-      SetConfigurationRequest.Mode mode) throws IOException;
+  RaftClientReply setConfiguration(SetConfigurationArguments arguments)
+      throws IOException;
 
   /** The same as setConfiguration(Arrays.asList(serversInNewConf), Arrays.asList(listenersInNewConf)). */
   default RaftClientReply setConfiguration(RaftPeer[] serversInNewConf, RaftPeer[] listenersInNewConf)
diff --git a/ratis-proto/src/main/proto/Raft.proto b/ratis-proto/src/main/proto/Raft.proto
index 2c149846..f6b38c14 100644
--- a/ratis-proto/src/main/proto/Raft.proto
+++ b/ratis-proto/src/main/proto/Raft.proto
@@ -406,15 +406,14 @@ message RaftClientReplyProto {
 
 // setConfiguration request
 message SetConfigurationRequestProto {
+  enum Mode {
+    SET_UNCONDITIONALLY = 0;
+    ADD = 1;
+  }
   RaftRpcRequestProto rpcRequest = 1;
   repeated RaftPeerProto peers = 2;
   repeated RaftPeerProto listeners = 3;
-  enum RaftConfigurationMode {
-    NORMAL = 0;
-    ADD = 1;
-  }
-
-  optional RaftConfigurationMode mode = 4;
+  optional Mode mode = 4;
 }
 
 // transfer leadership request

szetszwo

@codings-dan , thanks for the update. Some comments inlined.

szetszwo · 2022-06-23T17:54:02Z

ratis-client/src/main/java/org/apache/ratis/client/api/AdminApi.java

-  RaftClientReply setConfiguration(List<RaftPeer> serversInNewConf, List<RaftPeer> listenersInNewConf)
-      throws IOException;
-
-  /** The same as setConfiguration(Arrays.asList(serversInNewConf), Arrays.asList(listenersInNewConf)). */
-  default RaftClientReply setConfiguration(RaftPeer[] serversInNewConf, RaftPeer[] listenersInNewConf)
-      throws IOException {


We cannot remove these methods. Otherwise, it will become an incompatible change.

Thanks for pointing this out, I have fixed it

szetszwo · 2022-06-23T17:56:24Z

ratis-common/src/main/java/org/apache/ratis/protocol/SetConfigurationRequest.java

  private final List<RaftPeer> peers;
  private final List<RaftPeer> listeners;
+  private Mode mode;


Let's make it final in order to keep it immutable.

szetszwo · 2022-06-23T17:59:50Z

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

+         List<RaftPeer> peers = Stream.of(peersInNewConf, new ArrayList<>(current.getAllPeers()))
+            .flatMap(List :: stream).distinct().collect(Collectors.toList());


But the new peer may have a different priority. Should we allow it?

szetszwo

@codings-dan , thanks for the update. For the detailed comments, see https://issues.apache.org/jira/secure/attachment/13045825/658_review.patch

szetszwo · 2022-06-28T17:12:34Z

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

+         List<RaftPeer> peers = Stream.of(peersInNewConf, new ArrayList<>(current.getAllPeers()))
+            .flatMap(List :: stream).distinct().collect(Collectors.toList());


@codings-dan , But we use distinct() so that we are not sure whether the existing or the new server is used.

BTW, RaftConfigurationImpl#hasNoChange does not check listeners at all. It is a bug.

szetszwo · 2022-06-28T18:12:54Z

ratis-client/src/main/java/org/apache/ratis/client/api/AdminApi.java

@@ -31,24 +32,108 @@
 * such as setting raft configuration and transferring leadership.
 */
 public interface AdminApi {
+
+  class SetConfigurationArguments {


Let's move it to SetConfigurationRequest so that it can be used everywhere.

szetszwo

@codings-dan , thanks for the update. Some minor comments inlined.

szetszwo · 2022-06-29T16:57:04Z

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

-    if (!isStable() || conf.size() != newMembers.size()) {
+  boolean hasNoChange(Collection<RaftPeer> newMembers, Collection<RaftPeer> newListeners) {
+    if (!isStable() || conf.size() != newMembers.size()
+        || conf.getPeers(RaftPeerRole.LISTENER).size() != newListeners.size()) {
      return false;
    }
    for (RaftPeer peer : newMembers) {
      if (!conf.contains(peer.getId()) || conf.getPeer(peer.getId()).getPriority() != peer.getPriority()) {


This if-statement is no longer needed since the protos are compared below.

szetszwo · 2022-06-29T16:57:28Z

ratis-server/src/test/java/org/apache/ratis/server/impl/RaftReconfigurationBaseTest.java

@@ -23,6 +23,7 @@
 import org.apache.ratis.RaftTestUtil.SimpleMessage;
 import org.apache.ratis.client.RaftClient;
 import org.apache.ratis.client.RaftClientRpc;
+import org.apache.ratis.client.api.AdminApi;


This is unused.

I have remove the extra code

szetszwo · 2022-06-29T16:59:51Z

ratis-common/src/main/java/org/apache/ratis/protocol/SetConfigurationRequest.java

+    private List<RaftPeer> serversInNewConf;
+    private List<RaftPeer> listenersInNewConf;
+    private Mode mode;


Add final so that the class becomes immutable.

szetszwo · 2022-07-02T18:54:04Z

@codings-dan , there are some test failures. Please take a look.

szetszwo · 2022-07-04T18:17:00Z

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

-      if (!inConf.getRaftPeerProto().equals(peer.getRaftPeerProto())) {
+      if (inConf.getPriority() != peer.getPriority()) {


@codings-dan , I guess comparing protos has caused test failures. Why it is not working? It seems comparing protos is correct since it will compare all the fields.

I see now. The hostnames could be different

inConf:

id: "s0" address: "localhost:59232" dataStreamAddress: "localhost:59235" clientAddress: "localhost:59234" adminAddress: "localhost:59233"

peer :

id: "s0" address: "0.0.0.0:59232" clientAddress: "0.0.0.0:59234" adminAddress: "0.0.0.0:59233"

@codings-dan, Let's compare only the priority as you have already done. We should think about how to support updating hostnames and ports. Perhaps it needs a new mode.

@szetszwo Yean, the hostname could be different, so I only compare the priority.

We should think about how to support updating hostnames and ports

I will try, thanks!

szetszwo

+1 the change looks good.

SzyWilliam approved these changes Jun 20, 2022

View reviewed changes

szetszwo requested changes Jun 20, 2022

View reviewed changes

szetszwo reviewed Jun 23, 2022

View reviewed changes

szetszwo reviewed Jun 28, 2022

View reviewed changes

szetszwo reviewed Jun 29, 2022

View reviewed changes

codings-dan added 8 commits July 4, 2022 15:06

Support ADD mode to SetConfiguration

a33a97e

add SetConfigurationArguments class and Builder.

45da181

make code better

b355bc5

make code better

b97f410

refactor code and fix hasNoChange bug

5a7cbe4

make code better

9392224

fix ci error

cd19619

fix testNoChangeRequest error

1b2649c

codings-dan force-pushed the 1594 branch from 155c543 to 1b2649c Compare July 4, 2022 07:07

szetszwo reviewed Jul 4, 2022

View reviewed changes

szetszwo approved these changes Jul 4, 2022

View reviewed changes

szetszwo merged commit aeec309 into apache:master Jul 4, 2022

codings-dan deleted the 1594 branch July 5, 2022 07:56

szetszwo pushed a commit that referenced this pull request Jul 14, 2022

RATIS-1594. Support ADD mode to SetConfiguration (#658)

1effe6f

symious pushed a commit to symious/ratis that referenced this pull request Mar 5, 2024

RATIS-1594. Support ADD mode to SetConfiguration (apache#658)

7ed8923

		List<RaftPeer> peers = Stream.of(peersInNewConf, new ArrayList<>(current.getAllPeers()))
		.flatMap(List :: stream).distinct().collect(Collectors.toList());

		if (!inConf.getRaftPeerProto().equals(peer.getRaftPeerProto())) {
		if (inConf.getPriority() != peer.getPriority()) {

RATIS-1594. Support ADD mode to SetConfiguration #658

RATIS-1594. Support ADD mode to SetConfiguration #658

Conversation

codings-dan commented Jun 20, 2022

codings-dan commented Jun 20, 2022

SzyWilliam left a comment

Choose a reason for hiding this comment

szetszwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szetszwo commented Jun 21, 2022

szetszwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szetszwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szetszwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szetszwo commented Jul 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szetszwo left a comment

Choose a reason for hiding this comment