[GitHub] [ratis] szetszwo commented on a change in pull request #598: RATIS-1301. Support listener in ratis

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

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



##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftConfiguration.java
##########
@@ -36,9 +36,18 @@
    */
   RaftPeer getPeer(RaftPeerId id);
 
+  /**

Review comment:
       Let's update the javadoc of the class.
   ```
   +++ b/ratis-server-api/src/main/java/org/apache/ratis/server/RaftConfiguration.java
   @@ -25,9 +25,16 @@ import java.util.Collection;
    /**
     * A configuration is a subset of the members in a {@link org.apache.ratis.protocol.RaftGroup}.
     * The configuration of a cluster may change from time to time.
   + *
   + * In a configuration,
   + * - the peers are voting members such as LEADER, CANDIDATE and FOLLOWER;
   + * - the listeners are non-voting members.
   + *
     * This class captures the current configuration and the previous configuration of a cluster.
     *
     * The objects of this class are immutable.
   + *
   + * @see org.apache.ratis.proto.RaftProtos.RaftPeerRole
     */
    public interface RaftConfiguration {
   ```

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/PeerConfiguration.java
##########
@@ -36,23 +36,41 @@
  */
 class PeerConfiguration {
   private final Map<RaftPeerId, RaftPeer> peers;
+  private final Map<RaftPeerId, RaftPeer> listeners;

Review comment:
       Let's add some javadoc:
   ```
     /**
      * Peers are voting members such as LEADER, CANDIDATE and FOLLOWER
      * @see org.apache.ratis.proto.RaftProtos.RaftPeerRole
      */
     private final Map<RaftPeerId, RaftPeer> peers;
     /**
      * Listeners are non-voting members.
      * @see org.apache.ratis.proto.RaftProtos.RaftPeerRole#LISTENER
      */
     private final Map<RaftPeerId, RaftPeer> listeners;
   ```
   

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/PeerConfiguration.java
##########
@@ -36,23 +36,41 @@
  */
 class PeerConfiguration {
   private final Map<RaftPeerId, RaftPeer> peers;
+  private final Map<RaftPeerId, RaftPeer> listeners;
 
   PeerConfiguration(Iterable<RaftPeer> peers) {
+    this(peers, Collections.emptyList());
+  }
+
+  PeerConfiguration(Iterable<RaftPeer> peers, Iterable<RaftPeer> listeners) {

Review comment:
       We may add a utility method
   ```
     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<>();
       for(RaftPeer p : peers) {
         if (existing.containsKey(p.getId())) {
           throw new IllegalArgumentException("Failed to initialize " + name
               + ": Found " + p.getId() + " in existing peers " + existing);
         }
         final RaftPeer previous = map.putIfAbsent(p.getId(), p);
         if (previous != null) {
           throw new IllegalArgumentException("Failed to initialize " + name
               + ": Found duplicated ids " + p.getId() + " in " + peers);
         }
       }
       return Collections.unmodifiableMap(map);
     }
   ```
   Then, the constructor becomes:
   ```
   
     PeerConfiguration(Iterable<RaftPeer> peers, Iterable<RaftPeer> listeners) {
       this.peers = newMap(peers, "peers", Collections.emptyMap());
       this.listeners = newMap(listeners, "listeners", this.peers);
     }
   ```

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftConfigurationImpl.java
##########
@@ -145,6 +149,10 @@ boolean containsInConf(RaftPeerId peerId) {
     return conf.contains(peerId);
   }
 
+  boolean containsInListenerConf(RaftPeerId peerId) {

Review comment:
       Let's call it `containsListenerInConf`.
   
   Let's fix our terminology -- we only have Configuration or Conf, which has peers and listeners, but there is no such that as _Listener Conf_.  We do have listeners in a Conf.
   

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftConfigurationImpl.java
##########
@@ -172,6 +184,11 @@ boolean containsInBothConfs(RaftPeerId peerId) {
         (oldConf == null || containsInOldConf(peerId));
   }
 
+  boolean containsInBothListenerConfs(RaftPeerId peerId) {

Review comment:
       Rename it to `containsListenerInBothConfs`.

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftConfigurationImpl.java
##########
@@ -163,6 +171,10 @@ boolean containsInOldConf(RaftPeerId peerId) {
     return oldConf != null && oldConf.contains(peerId);
   }
 
+  boolean containsInOldListenerConf(RaftPeerId peerId) {

Review comment:
       Rename it to `containsListenerInOldConf`.

##########
File path: ratis-server-api/src/main/java/org/apache/ratis/server/RaftConfiguration.java
##########
@@ -36,9 +36,18 @@
    */
   RaftPeer getPeer(RaftPeerId id);
 
+  /**
+   * @return the listener corresponding to the given id;
+   *         or return null if the listener is not in this configuration.
+   */
+  RaftPeer getListener(RaftPeerId id);
+
   /** @return all the peers in the current configuration and the previous configuration. */
   Collection<RaftPeer> getAllPeers();
 
+  /** @return all the listeners in the current configuration and the previous configuration. */
+  Collection<RaftPeer> getListeners();

Review comment:
       Let's call it `getAllListeners()`.

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -353,6 +356,12 @@ private void startInitializing() {
     // do not start FollowerState
   }
 
+  private void startAsListener() {
+    setRole(RaftPeerRole.LISTENER, "startAsListener");
+    //TODO(codings-dan): ListenerState
+    lifeCycle.transition(RUNNING);
+  }
+

Review comment:
       Let's don't change RaftServerImpl in this pull request.  We will change it later.

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftConfigurationImpl.java
##########
@@ -196,6 +227,16 @@ public RaftPeer getPeer(RaftPeerId id) {
     return peers;
   }
 
+  @Override
+  public Collection<RaftPeer> getListeners() {
+    final Collection<RaftPeer> listeners = new ArrayList<>(conf.getListeners());
+    if (oldConf != null) {
+      oldConf.getListeners().stream().filter(p -> !listeners.contains(p))
+          .forEach(listeners::add);
+    }
+    return listeners;
+  }

Review comment:
       Let's refactor the code:
   ```
     @Override
     public Collection<RaftPeer> getAllPeers() {
       return getAll(PeerConfiguration::getPeers);
     }
   
     @Override
     public Collection<RaftPeer> getListeners() {
       return getAll(PeerConfiguration::getListeners);
     }
   
     private Collection<RaftPeer> getAll(Function<PeerConfiguration, Collection<RaftPeer>> getMethod) {
       final Collection<RaftPeer> peers = new ArrayList<>(getMethod.apply(conf));
       if (oldConf != null) {
         getMethod.apply(oldConf).stream()
             .filter(p -> !peers.contains(p))
             .forEach(peers::add);
       }
       return peers;
     }
   ```

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/PeerConfiguration.java
##########
@@ -66,10 +84,18 @@ RaftPeer getPeer(RaftPeerId id) {
     return peers.get(id);
   }
 
+  RaftPeer getListener(RaftPeerId id) {
+    return listeners.get(id);
+  }
+
   boolean contains(RaftPeerId id) {
     return peers.containsKey(id);
   }
 
+  boolean containsInListener(RaftPeerId id) {

Review comment:
       Just call it `containsListener`.

##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftConfigurationImpl.java
##########
@@ -186,6 +203,20 @@ public RaftPeer getPeer(RaftPeerId id) {
     return null;
   }
 
+  @Override
+  public RaftPeer getListener(RaftPeerId id) {
+    if (id == null) {
+      return null;
+    }
+    RaftPeer listener = conf.getListener(id);
+    if (listener != null) {
+      return listener;
+    } else if (oldConf != null) {
+      return oldConf.getListener(id);
+    }
+    return null;
+  }

Review comment:
       Let's refactor the code.
   ```
     @Override
     public RaftPeer getPeer(RaftPeerId id) {
       return get(id, (c, peerId) -> c.getPeer(id));
     }
   
     @Override
     public RaftPeer getListener(RaftPeerId id) {
       return get(id, (c, peerId) -> c.getListener(id));
     }
   
     private RaftPeer get(RaftPeerId id, BiFunction<PeerConfiguration, RaftPeerId, RaftPeer> getMethod) {
       if (id == null) {
         return null;
       }
       final RaftPeer peer = getMethod.apply(conf, id);
       if (peer != null) {
         return peer;
       } else if (oldConf != null) {
         return getMethod.apply(oldConf, id);
       }
       return null;
     }
   ```




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