Re: [PR] IGNITE-22564 Decouple metastore and cmg RAFT log from data partitions… [ignite-3]

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

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


##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/Loza.java:
##########
@@ -202,9 +196,14 @@ public <T extends RaftGroupService> CompletableFuture<T> startRaftGroupNode(
             PeersAndLearners configuration,
             RaftGroupListener lsnr,
             RaftGroupEventsListener eventsLsnr,
-            RaftServiceFactory<T> factory
+            RaftServiceFactory<T> factory,
+            RaftOptionsConfigurator storageConfigurator

Review Comment:
   Why did you choose a closure instead of pojo with clear fields?



##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/Loza.java:
##########
@@ -314,25 +325,29 @@ public <T extends RaftGroupService> CompletableFuture<T> startRaftGroupNodeAndWa
             RaftGroupListener lsnr,
             RaftGroupEventsListener eventsLsnr,
             RaftNodeDisruptorConfiguration disruptorConfiguration,
-            @Nullable RaftServiceFactory<T> factory
+            @Nullable RaftServiceFactory<T> factory,
+            RaftOptionsConfigurator storageConfigurator
     ) throws NodeStoppingException {
         if (!busyLock.enterBusy()) {
             throw new NodeStoppingException();
         }
 
         try {
-            CompletableFuture<T> startRaftServiceFuture = startRaftGroupNodeInternal(
+            RaftGroupOptions raftGroupOptions = RaftGroupOptions.defaults();
+
+            storageConfigurator.configure(raftGroupOptions);
+
+            raftGroupOptions.ownFsmCallerExecutorDisruptorConfig(disruptorConfiguration);

Review Comment:
   Why don't we reduce this method from Loza and add this setter to the raft option configuration closure (msRaftConfigurator)?



##########
modules/replicator/src/main/java/org/apache/ignite/internal/replicator/ReplicaManager.java:
##########
@@ -855,6 +862,8 @@ private RaftGroupOptions groupOptionsForPartition(boolean isVolatileStorage, Sna
 
         raftGroupOptions.commandsMarshaller(raftCommandsMarshaller);
 
+        partitionRaftConfigurator.configure(raftGroupOptions);

Review Comment:
   Only Loza do this in code above. Why do you think it ought to be done explicitly before Loza is called?



##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -405,6 +407,12 @@ public class IgniteImpl implements Ignite {
     /** Default log storage factory for raft. */
     private final LogStorageFactory logStorageFactory;
 
+    private final LogStorageFactory mslogStorageFactory;

Review Comment:
   Rename to ms**L**ogStorageFactory



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