[GitHub] [ignite-3] vldpyatkov commented on a diff in pull request #1397: IGNITE-18264 Add Peer index support

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

vldpyatkov commented on code in PR #1397:
URL: https://github.com/apache/ignite-3/pull/1397#discussion_r1044839860


##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/Loza.java:
##########
@@ -171,62 +168,41 @@ public void stop() throws Exception {
     @Override
     public CompletableFuture<RaftGroupService> prepareRaftGroup(
             ReplicationGroupId groupId,
-            Collection<String> nodeConsistentIds,
+            @Nullable Peer serverPeer,
+            PeersAndLearners configuration,
             Supplier<RaftGroupListener> lsnrSupplier
     ) throws NodeStoppingException {
-        return prepareRaftGroup(groupId, nodeConsistentIds, lsnrSupplier, RaftGroupOptions.defaults());
+        return prepareRaftGroup(groupId, serverPeer, configuration, lsnrSupplier, () -> noopLsnr, RaftGroupOptions.defaults());
     }
 
     @Override
     public CompletableFuture<RaftGroupService> prepareRaftGroup(
             ReplicationGroupId groupId,
-            Collection<String> nodeConsistentIds,
-            Collection<String> learnerConsistentIds,
+            @Nullable Peer serverPeer,
+            PeersAndLearners configuration,
             Supplier<RaftGroupListener> lsnrSupplier,
             Supplier<RaftGroupEventsListener> raftGrpEvtsLsnrSupplier
     ) throws NodeStoppingException {
-        return prepareRaftGroup(
-                groupId, nodeConsistentIds, learnerConsistentIds, lsnrSupplier, raftGrpEvtsLsnrSupplier, RaftGroupOptions.defaults()
-        );
-    }
-
-    /**
-     * Creates a raft group service providing operations on a raft group. If {@code nodes} contains the current node, then raft group starts
-     * on the current node.
-     *
-     * @param groupId Raft group id.
-     * @param peerConsistentIds Consistent IDs of Raft peers.
-     * @param lsnrSupplier Raft group listener supplier.
-     * @param groupOptions Options to apply to the group.
-     * @return Future representing pending completion of the operation.
-     * @throws NodeStoppingException If node stopping intention was detected.
-     */
-    public CompletableFuture<RaftGroupService> prepareRaftGroup(
-            ReplicationGroupId groupId,
-            Collection<String> peerConsistentIds,
-            Supplier<RaftGroupListener> lsnrSupplier,
-            RaftGroupOptions groupOptions
-    ) throws NodeStoppingException {
-        return prepareRaftGroup(groupId, peerConsistentIds, List.of(), lsnrSupplier, () -> noopLsnr, groupOptions);
+        return prepareRaftGroup(groupId, serverPeer, configuration, lsnrSupplier, raftGrpEvtsLsnrSupplier, RaftGroupOptions.defaults());
     }
 
     /**
-     * Creates a raft group service providing operations on a raft group. If {@code peerConsistentIds} or {@code learnerConsistentIds}
-     * contains the current node, then raft group starts on the current node.
+     * Optionally starts a Raft node and creates a Raft group service providing operations on a Raft group.
      *
-     * @param groupId Raft group id.
-     * @param peerConsistentIds Consistent IDs of Raft peers.
-     * @param learnerConsistentIds Consistent IDs of Raft learners.
+     * @param groupId Raft group ID.
+     * @param serverPeer Local peer that will host the Raft node. If {@code null} - no nodes will be started, but only the Raft client
+     *      service.
+     * @param configuration Peers and Learners of the Raft group.
      * @param lsnrSupplier Raft group listener supplier.
      * @param raftGrpEvtsLsnrSupplier Raft group events listener supplier.
      * @param groupOptions Options to apply to the group.
      * @return Future representing pending completion of the operation.
      * @throws NodeStoppingException If node stopping intention was detected.
      */
-    public CompletableFuture<RaftGroupService> prepareRaftGroup(
+    private CompletableFuture<RaftGroupService> prepareRaftGroup(
             ReplicationGroupId groupId,
-            Collection<String> peerConsistentIds,
-            Collection<String> learnerConsistentIds,
+            @Nullable Peer serverPeer,
+            PeersAndLearners configuration,
             Supplier<RaftGroupListener> lsnrSupplier,
             Supplier<RaftGroupEventsListener> raftGrpEvtsLsnrSupplier,

Review Comment:
   I think, both these suppliers are no longer useful. Because they had a sense, when the method determent, the RAFT node will start or not internally. Currently, it is regulated by serverPeer parameter.



##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/MetaStorageManager.java:
##########
@@ -157,24 +159,29 @@ public MetaStorageManager(
         this.storage = storage;
     }
 
-    private CompletableFuture<MetaStorageService> initializeMetaStorage(Collection<String> metaStorageNodes) {
+    private CompletableFuture<MetaStorageService> initializeMetaStorage(Set<String> metaStorageNodes) {
         ClusterNode thisNode = clusterService.topologyService().localMember();
 
-        if (metaStorageNodes.contains(thisNode.name())) {
+        PeersAndLearners configuration = PeersAndLearners.fromConsistentIds(metaStorageNodes);
+
+        Peer localPeer = configuration.peer(thisNode.name());
+
+        if (localPeer != null) {
             clusterService.topologyService().addEventHandler(new TopologyEventHandler() {
                 @Override
                 public void onDisappeared(ClusterNode member) {
                     metaStorageSvcFut.thenAccept(svc -> svc.closeCursors(member.id()));
                 }
             });
-        }
 
-        storage.start();
+            storage.start();
+        }
 
         try {
             CompletableFuture<RaftGroupService> raftServiceFuture = raftMgr.prepareRaftGroup(
                     INSTANCE,
-                    metaStorageNodes,
+                    localPeer,
+                    configuration,
                     () -> new MetaStorageListener(storage)

Review Comment:
   I see here you already know a raft listener required or not. Is the method with closure really required?



##########
modules/raft-api/src/main/java/org/apache/ignite/internal/raft/PeersAndLearners.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.raft;
+
+import static java.util.stream.Collectors.toUnmodifiableSet;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Class containing peers and learners of a Raft Group.
+ */
+public class PeersAndLearners {
+    @IgniteToStringInclude
+    private final Set<Peer> peers;
+
+    @IgniteToStringInclude
+    private final Set<Peer> learners;
+
+    private PeersAndLearners(Collection<Peer> peers, Collection<Peer> learners) {
+        this.peers = Set.copyOf(peers);
+        this.learners = Set.copyOf(learners);
+    }
+
+    /**
+     * Creates an instance using peers represented as their consistent IDs.
+     */
+    public static PeersAndLearners fromConsistentIds(Set<String> peerNames) {
+        return fromConsistentIds(peerNames, Set.of());
+    }
+
+    /**
+     * Creates an instance using peers and learners represented as their consistent IDs.
+     *
+     * <p>The main purpose of this method is to assign correct indices to Raft nodes when a peer and a learner are started on the same
+     * Ignite node. In this case {@code Peer} instances will have the same consistent IDs, but the learner will have an index equal to 1
+     * (having more than one peer and one learner running on the same node is currently prohibited).
+     */
+    public static PeersAndLearners fromConsistentIds(Set<String> peerNames, Set<String> learnerNames) {
+        Set<Peer> peers = peerNames.stream().map(Peer::new).collect(toUnmodifiableSet());
+
+        Set<Peer> learners = learnerNames.stream()
+                .map(name -> {
+                    int idx = peerNames.contains(name) ? 1 : 0;

Review Comment:
   Maybe we are defined these constants somewhere? (0 and 1 are not clear)



-- 
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: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org