[GitHub] [ignite-3] rpuch commented on a diff in pull request #1912: IGNITE-19231 Change thread pool for metastore raft group

["rpuch (via GitHub)" <gi...@apache.org>]

rpuch commented on code in PR #1912:
URL: https://github.com/apache/ignite-3/pull/1912#discussion_r1160735665


##########
modules/raft/src/integrationTest/java/org/apache/ignite/raft/jraft/core/ItNodeTest.java:
##########
@@ -3976,7 +3976,7 @@ private RaftGroupService createService(String groupId, TestPeer peer, NodeOption
 
         clusterService.start();
 
-        var service = new RaftGroupService(groupId, peer.getPeerId(), nodeOptions, rpcServer, nodeManager) {
+        var service = new RaftGroupService(groupId, peer.getPeerId(), nodeOptions, rpcServer, nodeManager, null) {

Review Comment:
   How about having two constructors in `RaftGroupService`, one identical to the old one (by parameters), another with this new added parameter, to avoid adding this `, null` all over the place?



##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/PartitionListener.java:
##########
@@ -175,6 +176,14 @@ public void onWrite(Iterator<CommandClosure<WriteCommand>> iterator) {
                 clo.result(null);
             } catch (IgniteInternalException e) {
                 clo.result(e);
+            } catch (CompletionException e) {
+                clo.result(e.getCause());
+            } catch (Throwable t) {

Review Comment:
   Shouldn't we rethrow it after logging? For example, after this change it would become a lot more difficult to notice that an `AssertionError` has happened.



##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/server/RaftServer.java:
##########
@@ -76,6 +77,26 @@ boolean startRaftNode(
             RaftGroupOptions groupOptions
     );
 
+    /**
+     * Starts a Raft group bound to this cluster node.
+     *
+     * @param nodeId Raft node ID.
+     * @param configuration Raft configuration.
+     * @param evLsnr Listener for group membership and other events.
+     * @param lsnr Listener for state machine events.
+     * @param groupOptions Options to apply to the group.
+     * @param ownFsmCallerExecutorDisruptorConfig Configuration own (not shared) striped disruptor for FSMCaller service of raft node.

Review Comment:
   ```suggestion
        * @param ownFsmCallerExecutorDisruptorConfig Configuration of own (not shared) striped disruptor for FSMCaller service of raft node.
   ```



##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/JRaftUtils.java:
##########
@@ -54,7 +54,7 @@ public final class JRaftUtils {
      * @return true if bootstrap success
      */
     public static boolean bootstrap(final BootstrapOptions opts) throws InterruptedException {
-        final NodeImpl node = new NodeImpl("bootstrap", new PeerId("127.0.0.1", 0));
+        final NodeImpl node = new NodeImpl("bootstrap", new PeerId("127.0.0.1", 0), null);

Review Comment:
   I suggest adding another constructor to `NodeImpl` to avoid this `, null` all over the place



##########
modules/raft-api/src/main/java/org/apache/ignite/internal/raft/RaftNodeDisruptorConfiguration.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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;
+
+/**
+ * Raft node disruptor configuration.
+ */
+public class RaftNodeDisruptorConfiguration {
+    private final String threadPostfix;
+
+    private final int stripes;
+
+    /**
+     * Constructor.
+     *
+     * @param threadPostfix Postfix the name of the disruptor threads.
+     * @param stripes Number of disruptor stripes.
+     */
+    public RaftNodeDisruptorConfiguration(String threadPostfix, int stripes) {
+        this.threadPostfix = threadPostfix;
+        this.stripes = stripes;
+    }
+
+    /**
+     * Return postfix the name of the disruptor threads.

Review Comment:
   ```suggestion
        * Return Disruptor threads' name postfix.
   ```



##########
modules/raft-api/src/main/java/org/apache/ignite/internal/raft/RaftNodeDisruptorConfiguration.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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;
+
+/**
+ * Raft node disruptor configuration.
+ */
+public class RaftNodeDisruptorConfiguration {
+    private final String threadPostfix;
+
+    private final int stripes;
+
+    /**
+     * Constructor.
+     *
+     * @param threadPostfix Postfix the name of the disruptor threads.

Review Comment:
   ```suggestion
        * @param threadPostfix Disruptor threads' name postfix.
   ```



##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/Loza.java:
##########
@@ -235,7 +236,36 @@ public <T extends RaftGroupService> CompletableFuture<T> startRaftGroupNode(
         }
 
         try {
-            return startRaftGroupNodeInternal(nodeId, configuration, lsnr, eventsLsnr, groupOptions, raftServiceFactory);
+            return startRaftGroupNodeInternal(nodeId, configuration, lsnr, eventsLsnr, groupOptions, raftServiceFactory, null);
+        } finally {
+            busyLock.leaveBusy();
+        }
+    }
+
+    @Override
+    public CompletableFuture<RaftGroupService> startRaftGroupNode(
+            RaftNodeId nodeId,
+            PeersAndLearners configuration,
+            RaftGroupListener lsnr,
+            RaftGroupEventsListener eventsLsnr,
+            RaftNodeDisruptorConfiguration ownFsmCallerExecutorDisruptorConfig

Review Comment:
   In this class, we have overloads of this method that accept `RaftGroupOptions`. `RaftGroupOptions` was added to aggregate all options that are specific for a group, and it seems that the FSM caller disruptor configuration you are adding can be just added to that class. As a result, we'll not need so many overloads. The new configuration class can be either merged into `RaftGroupOptions`, or included as a sub-structure (as it seems to be passed to `NodeImpl`).
   
   The only catch here is that these overloads (having `RaftGroupOptions` among parameters) are not present on `RaftServer`, but it seems that we can just add them there. By doing this, we'll be able to get rid of that cast to `Loza` in `TableManager` as a by-product.
   
   I consulted @sashapolo on this matter, he seems to support this suggestion. He asked to also add a TODO mentioning https://issues.apache.org/jira/browse/IGNITE-18273 to the new overloads in `RaftService` (if this suggestion gets accepted).



##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java:
##########
@@ -225,6 +227,9 @@ public class NodeImpl implements Node, RaftServerService {
      */
     private volatile int electionTimeoutCounter;
 
+    /** Configuration own striped disruptor for FSMCaller service of raft node, {@code null} means use shared disruptor. */

Review Comment:
   ```suggestion
       /** Configuration of own striped disruptor for FSMCaller service of raft node, {@code null} means use shared disruptor. */
   ```



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