[GitHub] [ratis] szetszwo commented on a change in pull request #614: RATIS-1540. Remove old PeerConfiguration API and change SetConfiguration

[GitBox <gi...@apache.org>]

szetszwo commented on a change in pull request #614:
URL: https://github.com/apache/ratis/pull/614#discussion_r816767336



##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/AdminApi.java
##########
@@ -30,14 +30,25 @@
  * such as setting raft configuration and transferring leadership.
  */
 public interface AdminApi {
-  /** Set the configuration request to the raft service. */
+

Review comment:
       Add default implementation to the old API and add javadoc.
   ```
     /** The same as setConfiguration(serversInNewConf, Collections.emptyList()). */
     default RaftClientReply setConfiguration(List<RaftPeer> serversInNewConf) throws IOException {
       return setConfiguration(serversInNewConf, Collections.emptyList());
     }
   ```

##########
File path: ratis-client/src/main/java/org/apache/ratis/client/impl/AdminImpl.java
##########
@@ -38,13 +39,19 @@
 
   @Override
   public RaftClientReply setConfiguration(List<RaftPeer> peersInNewConf) throws IOException {
+    return setConfiguration(peersInNewConf, Collections.emptyList());
+  }

Review comment:
       Remove the old API since it has a default.

##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/AdminApi.java
##########
@@ -30,14 +30,25 @@
  * such as setting raft configuration and transferring leadership.
  */
 public interface AdminApi {
-  /** Set the configuration request to the raft service. */
+
   RaftClientReply setConfiguration(List<RaftPeer> serversInNewConf) throws IOException;
 
+  /** Set the configuration request to the raft service. */
+  RaftClientReply setConfiguration(List<RaftPeer> serversInNewConf, List<RaftPeer> listenersInNewConf)
+      throws IOException;
+
   /** The same as setConfiguration(Arrays.asList(serversInNewConf)). */
-  default RaftClientReply setConfiguration(RaftPeer[] serversInNewConf) throws IOException {
+  default RaftClientReply setConfiguration(RaftPeer[] serversInNewConf)
+      throws IOException {

Review comment:
       Revert whitespace change.

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/PeerConfiguration.java
##########
@@ -47,8 +47,10 @@
   private final Map<RaftPeerId, RaftPeer> listeners;
 
   static Map<RaftPeerId, RaftPeer> newMap(Iterable<RaftPeer> peers, String name, Map<RaftPeerId, RaftPeer> existing) {
-    Objects.requireNonNull(peers, () -> name + " == null");
     final Map<RaftPeerId, RaftPeer> map = new HashMap<>();
+    if (Objects.isNull(peers)) {
+      return map;
+    }

Review comment:
       Since peers cannot be null but listeners can, let's check listeners outside.
   ```
      PeerConfiguration(Iterable<RaftPeer> peers, Iterable<RaftPeer> listeners) {
        this.peers = newMap(peers, "peers", Collections.emptyMap());
   -    this.listeners = newMap(listeners, "listeners", this.peers);
   +    this.listeners = Optional.ofNullable(listeners)
   +        .map(l -> newMap(listeners, "listeners", this.peers))
   +        .orElseGet(Collections::emptyMap);
      }
   ```

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/raftlog/LogProtoUtils.java
##########
@@ -200,8 +200,11 @@ public static RaftConfiguration toRaftConfiguration(LogEntryProto entry) {
     Preconditions.assertTrue(entry.hasConfigurationEntry());
     final RaftConfigurationProto proto = entry.getConfigurationEntry();
     final List<RaftPeer> conf = ProtoUtils.toRaftPeers(proto.getPeersList());
-    final List<RaftPeer> oldConf = proto.getOldPeersCount() == 0? null
+    final List<RaftPeer> listener = ProtoUtils.toRaftPeers(proto.getListenersList());
+    final List<RaftPeer> oldConf = proto.getOldPeersCount() == 0 ? null
         : ProtoUtils.toRaftPeers(proto.getOldPeersList());
-    return ServerImplUtils.newRaftConfiguration(conf, entry.getIndex(), oldConf);
+    final List<RaftPeer> oldListener = proto.getOldListenersCount() == 0 ? null
+        : ProtoUtils.toRaftPeers(proto.getOldListenersList());
+    return ServerImplUtils.newRaftConfiguration(conf, listener, entry.getIndex(), oldConf, oldListener);

Review comment:
       Let's simply call ProtoUtils.toRaftPeer(..) directly.
   ```
       final List<RaftPeer> listener = ProtoUtils.toRaftPeers(proto.getListenersList());
       final List<RaftPeer> oldConf = ProtoUtils.toRaftPeers(proto.getOldPeersList());
       final List<RaftPeer> oldListener = ProtoUtils.toRaftPeers(proto.getOldListenersList());
       return ServerImplUtils.newRaftConfiguration(conf, listener, entry.getIndex(), oldConf, oldListener);
   ```
   

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java
##########
@@ -362,22 +362,27 @@ PendingRequest startSetConfiguration(SetConfigurationRequest request) {
     Preconditions.assertTrue(running && !inStagingState());
 
     final List<RaftPeer> peersInNewConf = request.getPeersInNewConf();
+    final List<RaftPeer> listenersInNewConf = request.getListenersInNewConf();
     final Collection<RaftPeer> peersToBootStrap = server.getRaftConf().filterNotContainedInConf(peersInNewConf);
+    final Collection<RaftPeer> listenersToBootStrap =
+        server.getRaftConf().filterListenerNotContainedInConf(listenersInNewConf);
 
     // add the request to the pending queue
     final PendingRequest pending = pendingRequests.addConfRequest(request);
 
     ConfigurationStagingState configurationStagingState = new ConfigurationStagingState(
-        peersToBootStrap, new PeerConfiguration(peersInNewConf));
+        peersToBootStrap, listenersToBootStrap, new PeerConfiguration(peersInNewConf, listenersInNewConf));
     Collection<RaftPeer> newPeers = configurationStagingState.getNewPeers();
+    Collection<RaftPeer> newListeners = configurationStagingState.getNewListeners();
     // set the staging state
     this.stagingState = configurationStagingState;
 
-    if (newPeers.isEmpty()) {
+    if (newPeers.isEmpty() && newListeners.isEmpty()) {
       applyOldNewConf();
     } else {
       // update the LeaderState's sender list
       addAndStartSenders(newPeers);
+      addAndStartListeners(newListeners);

Review comment:
       Let's don't change startSetConfiguration and other code in LeaderStateImpl yet since it needs more thought -- Some old peers could become a listener in the new conf.  Similarly, some old listeners could become a regular peer in the new conf.  We will change it in the next pull request.

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftConfigurationImpl.java
##########
@@ -63,10 +63,6 @@ Builder setConf(PeerConfiguration conf) {
       return this;
     }
 
-    Builder setConf(Iterable<RaftPeer> peers) {
-      return setConf(new PeerConfiguration(peers));
-    }
-

Review comment:
       Let's keep this builder method since ServerState has a valid use of it.  Then, we don't have to change ServerState, DataStreamBaseTest and some other code.

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/ServerImplUtils.java
##########
@@ -70,11 +70,15 @@ private static RaftServerProxy newRaftServer(
     return proxy;
   }
 
-  public static RaftConfiguration newRaftConfiguration(List<RaftPeer> conf, long index, List<RaftPeer> oldConf) {
+  public static RaftConfiguration newRaftConfiguration(List<RaftPeer> conf, List<RaftPeer> listener,
+      long index, List<RaftPeer> oldConf, List<RaftPeer> oldListener) {
     final RaftConfigurationImpl.Builder b = RaftConfigurationImpl.newBuilder()
-        .setConf(conf)
+        .setConf(conf, listener)
         .setLogEntryIndex(index);
-    Optional.ofNullable(oldConf).filter(p -> p.size() > 0).ifPresent(b::setOldConf);
+    if (Optional.ofNullable(oldListener).filter(p -> p.size() > 0).isPresent()
+        || Optional.ofNullable(oldConf).filter(p -> p.size() > 0).isPresent()) {
+      b.setOldConf(oldConf, oldListener);
+    }

Review comment:
       Now, we can assume that oldConf and oldListener are non-null.
   ```
       if (!oldConf.isEmpty() || !oldListener.isEmpty()) {
         b.setOldConf(oldConf, oldListener);
       }
   ```

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/PeerConfiguration.java
##########
@@ -63,10 +65,6 @@
     return Collections.unmodifiableMap(map);
   }
 
-  PeerConfiguration(Iterable<RaftPeer> peers) {
-    this(peers, Collections.emptyList());
-  }
-

Review comment:
       Let's keep this method since listeners are optional.  We should not require to pass a empty list everywhere.

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftConfigurationImpl.java
##########
@@ -288,6 +284,11 @@ public long getLogEntryIndex() {
     return peers.stream().filter(p -> !containsInConf(p.getId())).collect(Collectors.toList());
   }
 
+  /** @return the peers which are not contained in conf. */
+  Collection<RaftPeer> filterListenerNotContainedInConf(List<RaftPeer> listeners) {
+    return listeners.stream().filter(p -> !containsListenerInConf(p.getId())).collect(Collectors.toList());
+  }
+

Review comment:
       Let's add this method later since we need to thing about the new logic.




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