[GitHub] [ignite-3] kgusakov commented on a diff in pull request #1383: IGNITE-18265 Update Raft packages structure

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

kgusakov commented on code in PR #1383:
URL: https://github.com/apache/ignite-3/pull/1383#discussion_r1036174962


##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -769,7 +775,7 @@ private void updateAssignmentInternal(ConfigurationNotificationEvent<byte[]> ass
                                         );
 
                                         try {
-                                            raftMgr.startRaftGroupNode(
+                                            ((Loza) raftMgr).startRaftGroupNode(

Review Comment:
   Am I right, that in a future we want to use `RaftManager` interface instead? If yes, could you add it to IGNITE-18273 or create another one issue to commit this intention?



##########
modules/affinity/pom.xml:
##########
@@ -33,39 +33,17 @@
     <version>3.0.0-SNAPSHOT</version>
 
     <dependencies>
-        <dependency>

Review Comment:
   Is it just cleanup of legacy unactual dependencies? Just want to clarify.



##########
modules/metastorage-client/src/integrationTest/java/org/apache/ignite/internal/metastorage/client/ItMetaStorageServiceTest.java:
##########
@@ -908,38 +895,32 @@ public void testCursorsCleanup() throws Exception {
             return cursor;
         });
 
-        List<Peer> peers = List.of(new Peer(cluster.get(0).topologyService().localMember().name()));
+        List<String> peers = List.of(cluster.get(0).topologyService().localMember().name());
 
-        RaftGroupService metaStorageRaftGrpSvc = RaftGroupServiceImpl.start(
-                INSTANCE,
-                cluster.get(1),
-                FACTORY,
-                10_000,
-                peers,
-                true,
-                200,
-                executor
-        ).get(3, TimeUnit.SECONDS);
+        MetaStorageService metaStorageSvc2 = raftManagers.get(1)
+                .startRaftGroupService(INSTANCE, peers, List.of())
+                .thenApply(service -> new MetaStorageServiceImpl(service, NODE_ID_1, NODE_ID_1))
+                .get(3, TimeUnit.SECONDS);
 
-        try {
-            MetaStorageService metaStorageSvc2 = new MetaStorageServiceImpl(metaStorageRaftGrpSvc, NODE_ID_1, NODE_ID_1);
+        Cursor<Entry> cursorNode0 = metaStorageSvc.range(EXPECTED_RESULT_ENTRY.key(), null);
 
-            Cursor<Entry> cursorNode0 = metaStorageSvc.range(EXPECTED_RESULT_ENTRY.key(), null);
+        assertTrue(cursorNode0.hasNext());
 
-            Cursor<Entry> cursor2Node0 = metaStorageSvc.range(EXPECTED_RESULT_ENTRY.key(), null);
+        Cursor<Entry> cursor2Node0 = metaStorageSvc.range(EXPECTED_RESULT_ENTRY.key(), null);
 
-            final Cursor<Entry> cursorNode1 = metaStorageSvc2.range(EXPECTED_RESULT_ENTRY.key(), null);
+        assertTrue(cursor2Node0.hasNext());
 
-            metaStorageSvc.closeCursors(NODE_ID_0).get();
+        Cursor<Entry> cursorNode1 = metaStorageSvc2.range(EXPECTED_RESULT_ENTRY.key(), null);
 
-            assertThrows(NoSuchElementException.class, () -> cursorNode0.iterator().next());
+        assertTrue(cursorNode1.hasNext());
 
-            assertThrows(NoSuchElementException.class, () -> cursor2Node0.iterator().next());
+        metaStorageSvc.closeCursors(NODE_ID_0).get();
 
-            assertEquals(EXPECTED_RESULT_ENTRY, (cursorNode1.iterator().next()));
-        } finally {
-            metaStorageRaftGrpSvc.shutdown();

Review Comment:
   Shouldn't we leave this shutdown in place? I know, that it is "no-op" at the moment, but it is a part of RaftGroupService contract and can be extended by any logic in future.



##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/MockedStructuresTest.java:
##########
@@ -511,7 +508,7 @@ private TableManager createTableManager() {
                 workDir,
                 msm,
                 schemaManager,
-                view -> new LocalLogStorageFactory(),
+                null,

Review Comment:
   Are we sure, that it isn't dangerous? Maybe ask @rpuch ?



##########
modules/runner/build.gradle:
##########
@@ -32,6 +32,7 @@ dependencies {
     implementation project(':ignite-api')
     implementation project(':ignite-vault')
     implementation project(':ignite-network')
+    implementation project(':ignite-raft-api')

Review Comment:
   What about the same changes in pom.xml?



##########
modules/table/build.gradle:
##########
@@ -33,7 +33,7 @@ dependencies {
     implementation project(':ignite-network-api')
     implementation project(':ignite-raft')
     implementation project(':ignite-replicator')
-    implementation project(':ignite-raft-client')
+    implementation project(':ignite-raft-api')

Review Comment:
   pom.xml?



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