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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
