IGNITE-22315 Make raft-client starting only once and only with raft-client and replica together #3956

JAkutenshi · 2024-06-19T16:17:45Z

JIRA Ticket: IGNITE-22315 | Make raft-client starting only once and only with raft-client and replica together

The goal

The goal of the ticket is to remove excessive raft-clients starting.

The reason

If a local node is in assignments, then and only then we should start a whole replication group, that consists of raft-node, raft-client and replica itself. Every entity is a single copy per group and neither should be started outside of the group. Before this ticket every partition started a raft-client.

The solution

Related completion stage that create raft-client is removed.
Tests are adjusted for such behavior and check that raft-clients started and updated only on replication group and only.
ReplicaManager#startReplica chains all raft-entities starting together with replica starting to a common future.
Replica manager now handle with replicas starting executor. The motivation do not grab it from table manager is better control of the lifecycle: the executor starts inside replica manager and there is terminated.
TableManager#startPartitionAndStartClient is refactored due to simplicity of the code.

Abandoned PR due to unstable tests flaking is closed

# Conflicts: # modules/replicator/src/main/java/org/apache/ignite/internal/replicator/ReplicaManager.java # modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java

…on waiting of restart

kgusakov · 2024-06-24T10:33:47Z

modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java

+        if (localMemberAssignment == null) {
+            // (0) in case if node not in the assignments
+            return nullCompletedFuture();
+        }


Make an empty line here, pls

kgusakov · 2024-06-24T10:59:27Z

modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItBuildIndexTest.java

+                    // TODO: will be replaced with replica usage in https://issues.apache.org/jira/browse/IGNITE-22218
+                    RaftGroupService raftGroupService;
+                    try {
+                        raftGroupService = internalTable.tableRaftService().partitionRaftGroupService(partitionId);


Why do we need this try-catch now? Could you place a comment, why the simple continue is enough? Shouldn't we write the log with warn?

the test's logic is go through every partition and try to get raft client from TableRaftService if presented. The method TableRaftService#partitionRaftGroupService says in javadoc: IgniteInternalException if partition can't be found.. So, the only way for now safely check that there no raft client is to catch the exception. The good news is this code is already removed in the next related PR, so, I guess we can leave it now?

...egrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteInMemoryNodeRestartTest.java

kgusakov · 2024-06-24T11:24:28Z

...rc/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java

+                                    } catch (IgniteInternalException e) {
+                                        return false;
+                                    }
+                                    return raftClient != null;


Please add empty lines here and there :) according to the recommendation "The source code should be broken into minimal semantic units and all such units must be separated by one empty line. To simplify the recognition of semantic units, every line of the source code should be separated by one empty line" from the https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-SemanticUnits

Rewrote it a little

kgusakov · 2024-06-24T11:31:55Z

modules/table/src/testFixtures/java/org/apache/ignite/distributed/ItTxTestCluster.java

@@ -713,7 +713,7 @@ public TableViewInternal startTable(String tableName, SchemaDescriptor schemaDes
                                        storageIndexTracker,
                                        completedFuture(listener)
                                );
-                            } catch (NodeStoppingException e) {
+                            } catch (Throwable e) {


Looks like it can be not only node stopping now, but any NPE for example and etc.

The try-catch is removed it looks like excessive there.

# Conflicts: # modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java

...egrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteInMemoryNodeRestartTest.java

modules/replicator/src/main/java/org/apache/ignite/internal/replicator/ReplicaManager.java

...egrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteInMemoryNodeRestartTest.java

modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java

sanpwc · 2024-06-28T06:59:33Z

modules/table/src/testFixtures/java/org/apache/ignite/distributed/ItTxTestCluster.java

@@ -713,9 +711,6 @@ public TableViewInternal startTable(String tableName, SchemaDescriptor schemaDes
                                        storageIndexTracker,
                                        completedFuture(listener)
                                );
-                            } catch (NodeStoppingException e) {


Why?
Do you mean that will through unchecked exception instead? Which one if true? Or there's no longer network communication on replica creation and thus it's not possible to get NodeStoppingException?

There deprecated startReplica is used that doesn't throw NodeStoppingException explicitly == there are no calls that throw the checked exception, but the true one startReplica does, but isn't used in the test and will be fixed in IGNITE-22373 | Delete startReplica overloads from ReplicaManager

...rc/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java

…gAssignmentEvent` branch

…balance` (don't know actually why)

…mentEvent` branch

…case

…tract the common method

…dition fixed

…updatePartitionClients`

IGNITE-22315 Init

b29aad4

JAkutenshi marked this pull request as draft June 19, 2024 16:17

JAkutenshi added 8 commits June 19, 2024 22:51

IGNITE-22315 BuildIndexTest fix + checkstyle fix

27e401a

IGNITE-22315 start replica with raft chaining

e467d39

IGNITE-22315 solePartitionAssignmentsContain fix

9b84bb6

IGNITE-22315 TableManager start replica chaining refactoring

59a8291

IGNITE-22315 pmd fix

23ec8b6

Merge branch 'refs/heads/main' into ignite-22315-redone

9343f15

# Conflicts: # modules/replicator/src/main/java/org/apache/ignite/internal/replicator/ReplicaManager.java # modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java

IGNITE-22315 Main merge with Kirill's changes

09fab58

IGNITE-22315 Remove unintentional repeated tests

29e22d9

JAkutenshi changed the title ~~IGNITE-22315 [Draft] [Fresh redone branch] ]Make raft-client starting only once and only with raft-client and replica together~~ IGNITE-22315 Make raft-client starting only once and only with raft-client and replica together Jun 21, 2024

JAkutenshi marked this pull request as ready for review June 21, 2024 18:05

JAkutenshi added 2 commits June 22, 2024 00:38

IGNITE-22315 inMemoryNodeRestartNotLeader return test and fix assert …

4afcdfc

…on waiting of restart

Merge branch 'refs/heads/main' into ignite-22315-redone

bb97122

kgusakov suggested changes Jun 24, 2024

View reviewed changes

IGNITE-22315 review fix

db58d53

JAkutenshi requested a review from kgusakov June 24, 2024 16:09

Merge branch 'refs/heads/main' into ignite-22315-redone

ad772fd

# Conflicts: # modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java

sanpwc reviewed Jun 28, 2024

View reviewed changes

JAkutenshi added 11 commits July 1, 2024 19:31

IGNITE-22567 review fixes

4f27636

IGNITE-22567 Temporal isReplicaStarted check on `handleChangePendin…

3353ee5

…gAssignmentEvent` branch

IGNITE-22567 using pendingAssignments check before `changePeersOnRe…

7ddd40a

…balance` (don't know actually why)

IGNITE-22315 double-check asserts

601c00e

IGNITE-22315 assert fix

b754d08

IGNITE-22315 return isReplicaStarted for `handleChangePendingAssign…

0eb4061

…mentEvent` branch

IGNITE-22315 remove assert inside updatePartitionClients non-local …

7fbd1c0

…case

IGNITE-22315 isReplicaStarted isn't using now

f0eb3f8

IGNITE-22315 PMD fix

e068e55

IGNITE-22315 add additional log information in the assertion

b3036df

IGNITE-22315 add additional log information in both assertions and ex…

54c137d

…tract the common method

JAkutenshi and others added 12 commits July 3, 2024 14:02

IGNITE-22315 isNodeInReducedStableAndPendingAssignmentsUnionSet con…

b681255

…dition fixed

IGNITE-22315 Add busylock on update

4dedc25

IGNITE-22315 Added a note about busy lock in stopAsync

5663d9d

IGNITE-22315 Checks that local mode may be a primary replica inside `…

1d982ad

…updatePartitionClients`

IGNITE-22315 fix timestamp for getPrimaryReplica using

532bf5c

IGNITE-22315 check primary replica on pending assignments handling

4171117

IGNITE-22315 more certain assertions' logs

797ce61

IGNITE-22315 add busy lock on updatePartitionClients

accf73e

Merge branch 'refs/heads/main' into ignite-22315-redone

b387feb

IGNITE-22315 After merge compilation fixes

c7bacd6

IGNITE-22315 Debug LOG.info are removed

23d42c7

Merge branch 'refs/heads/main' into ignite-22315-redone

968e8f7

sanpwc approved these changes Jul 8, 2024

View reviewed changes

IGNITE-22315 merge main

150a22b

sanpwc merged commit 84b760b into apache:main Jul 9, 2024
1 check passed

sanpwc deleted the ignite-22315-redone branch July 9, 2024 05:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGNITE-22315 Make raft-client starting only once and only with raft-client and replica together #3956

IGNITE-22315 Make raft-client starting only once and only with raft-client and replica together #3956

JAkutenshi commented Jun 19, 2024 •

edited

Loading

kgusakov Jun 24, 2024

JAkutenshi Jun 24, 2024

kgusakov Jun 24, 2024

JAkutenshi Jun 24, 2024 •

edited

Loading

kgusakov Jun 24, 2024

JAkutenshi Jun 24, 2024 •

edited

Loading

kgusakov Jun 24, 2024

JAkutenshi Jun 24, 2024

sanpwc Jun 28, 2024

JAkutenshi Jul 1, 2024

IGNITE-22315 Make raft-client starting only once and only with raft-client and replica together #3956

IGNITE-22315 Make raft-client starting only once and only with raft-client and replica together #3956

Conversation

JAkutenshi commented Jun 19, 2024 • edited Loading

The goal

The reason

The solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAkutenshi Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAkutenshi Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAkutenshi commented Jun 19, 2024 •

edited

Loading

JAkutenshi Jun 24, 2024 •

edited

Loading

JAkutenshi Jun 24, 2024 •

edited

Loading