IGNITE-22233 Zone replica listener #3931

kgusakov · 2024-06-16T21:25:01Z

Thank you for submitting the pull request.

To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:

The Review Checklist

Formal criteria: TC status, codestyle, mandatory documentation. Also make sure to complete the following:
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Design: new code conforms with the design principles of the components it is added to.
Patch quality: patch cannot be split into smaller pieces, its size must be reasonable.
Code quality: code is clean and readable, necessary developer documentation is added if needed.
Tests code quality: test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.

Notes

Apache Ignite Coding Guidelines

modules/replicator/src/main/java/org/apache/ignite/internal/replicator/message/TableAware.java

...les/table/src/integrationTest/java/org/apache/ignite/distributed/ReplicaUnavailableTest.java

sanpwc · 2024-06-25T13:55:09Z

...n/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java

+     * @param replica Table replica
+     * @return Future, which will be completed when operation done
+     */
+    public CompletableFuture<Void> addTableReplica(ZonePartitionId zonePartitionId, TablePartitionId replicationGroupId, Replica replica) {


As discussed,

I'd consider moving the method to the Replica itself.

Change Replica param to Listener.

Rename given method to addTableReplicaListener or addTableRequestProcessor.

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

sanpwc · 2024-06-25T14:07:18Z

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


-        startReplica(replicaGrpId, storageIndexTracker, newReplicaListenerFut);
+        CompletableFuture<ReplicaListener> newReplicaListenerFut = resultFuture.thenApply(createListener);


Missed that previously createListener is rather confusing naming. I'd rather use common supplier suffix.

Yep, but actually it is not a supplier, but the function with an argument instead.

sanpwc · 2024-06-25T14:28:00Z

...replicator/src/main/java/org/apache/ignite/internal/replicator/ZonePartitionReplicaImpl.java

@@ -50,7 +53,28 @@ public TopologyAwareRaftGroupService raftClient() {

    @Override
    public CompletableFuture<ReplicaResult> processRequest(ReplicaRequest request, String senderId) {
-        return listener.invoke(request, senderId);
+        if (!(request instanceof TableAware)) {


What kind of requests are supposed to be processed here besides TableAware ones, Txn-related and SafeTime propagation related ones, right? Or there are more?
If two mentioned above - could you please create a tickets and add todos here?

sanpwc · 2024-06-25T14:30:31Z

...replicator/src/main/java/org/apache/ignite/internal/replicator/ZonePartitionReplicaImpl.java

+            ReplicationGroupId replicationGroupId = request.groupId();
+
+            // TODO: https://issues.apache.org/jira/browse/IGNITE-22522 Refine this code when the zone based replication will done.
+            if (replicationGroupId instanceof  TablePartitionId) {


Could you please name requests that will use TablePartitionId for now?

Not sure, that I understood the question: in general in the main all requests, besides the idle safe time request, I suppose. But in the collocation feature branch I think it is not truth.

sanpwc · 2024-06-25T14:37:16Z

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

@@ -410,6 +412,8 @@ public class TableManager implements IgniteTablesInternal, IgniteComponent {

    private final IndexMetaStorage indexMetaStorage;

+    private PartitionReplicaLifecycleManager partitionReplicaLifecycleManager;


I don't think that TableManager should depend on PartitionReplciaLifecycleManager. Already existing replicaManager dependency should be enough. Meaning that TableManager should retrieve Replica from ReplicaManager and addTableProcessor to the replica directly. Basically it's a continuation of our discussion on collocation sync.

Done, but we still need the module dependency, because I need to check the PartitionReplicaLifecycleManager.ENABLED flag

sanpwc · 2024-06-25T14:39:05Z

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

@@ -1004,7 +1011,7 @@ private CompletableFuture<Void> startPartitionAndStartClient(
                        MvTableStorage mvTableStorage = internalTable.storage();

                        try {
-                            var ret = replicaMgr.startReplica(
+                            return replicaMgr.startReplica(


Please add the "code removal todo", it won't be necessary to startReplica after full migration to the zone based ones. Currently, yes, we will add it both to replicas directly and populate aggregated one with table processor.

sanpwc · 2024-06-25T15:00:11Z

...grationTest/java/org/apache/ignite/internal/partition/replicator/ItReplicaLifecycleTest.java

+
+            assertDoesNotThrow(() -> keyValueView.put(null, 1L, 200));
+
+            // Actually we are testing not the fair put value, but the hardcoded one from temporary noop replica listener


That should not be true any longer. We only skip the raft replication part, but we will retrieve previously inserted data. Because of this, I believe that we may verify random value insertion/retrieval.

Removed the comment. What about the values - it's already has two differents values check 100 vs 200. Do you think that we really has fair randomization here?

sanpwc · 2024-07-22T11:07:08Z

...src/main/java/org/apache/ignite/internal/partition/replicator/ZonePartitionRaftListener.java


 /**
 * RAFT listener for the zone partition.
 */
 public class ZonePartitionRaftListener implements RaftGroupListener {
+    private static final IgniteLogger LOG = Loggers.forClass(ZonePartitionReplicaImpl.class);


Wrong class.

sanpwc · 2024-07-22T11:09:30Z

...src/main/java/org/apache/ignite/internal/partition/replicator/ZonePartitionRaftListener.java

@@ -37,7 +50,35 @@ public void onRead(Iterator<CommandClosure<ReadCommand>> iterator) {



I'd rather add following implementation for onRead()

iterator.forEachRemaining((CommandClosure<? extends ReadCommand> clo) -> { Command command = clo.command(); assert false : "No read commands expected, [cmd=" + command + ']'; });

just like the one we have in PartitionListener

sanpwc · 2024-07-22T11:20:52Z

.../main/java/org/apache/ignite/internal/partition/replicator/ZonePartitionReplicaListener.java

+    // TODO: https://issues.apache.org/jira/browse/IGNITE-22624 await for the table replica listener if needed.
+    private final Map<TablePartitionId, ReplicaListener> replicas = new ConcurrentHashMap<>();
+
+    private final RaftGroupService raftClient;


Please use RaftCommandRunner.

sanpwc · 2024-07-22T12:22:23Z

.../main/java/org/apache/ignite/internal/partition/replicator/ZonePartitionReplicaListener.java

+            // TODO: https://issues.apache.org/jira/browse/IGNITE-22621 implement zone-based transaction storage
+            //  and txn messages processing
+            if (request instanceof TxFinishReplicaRequest) {
+                TxFinishReplicaRequest txFinishReplicaRquest = (TxFinishReplicaRequest) request;


Just in case. type in Rquest.

sanpwc · 2024-07-22T12:25:09Z

.../main/java/org/apache/ignite/internal/partition/replicator/ZonePartitionReplicaListener.java

+                        .get(txFinishReplicaRquest.commitPartitionId().asTablePartitionId())
+                        .invoke(requestForTableListener, senderId);
+            } else {
+                LOG.debug("Non table request is not supported by the zone partition yet " + request);


So, we will see such messages for SafeTimePropagationRequest, right? Any other? I believe I've already asked this before.

Yep, only the SafeTimePropagationRequest I guess.

sanpwc · 2024-07-22T13:58:25Z

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

+     * @return Replica's listener.
+     */
+    @Deprecated(forRemoval = true)
+    ReplicaListener listener();


Assuming that it's a tmp solution, how are you going to add table processor to the ZoneReplcia?
I mean that currently it's

replicaMgr.replica(new ZonePartitionId(zoneId, partId)) .thenAcceptAsync(zoneReplica -> ((ZonePartitionReplicaListener) zoneReplica.listener()).addTableReplicaListener( new TablePartitionId(tableId, partId), createListener ), ioExecutor );

What is an expected solution to substitute aforementioned one?

The tricky part is the the fact, that the ZonePartitionReplicaImpl is the part of replicator module and I can't use the ZonePartitionReplicaListener cast or smth else inside it. So, I have no better ideas for now, to be honest

sanpwc · 2024-07-22T14:45:22Z

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

@@ -635,6 +645,133 @@ public CompletableFuture<Void> startAsync(ComponentContext componentContext) {
        });
    }

+    private CompletableFuture<Boolean> initTableFsmOnTableCreate(CreateTableEventParameters parameters) {


Naming seems confusing to me. I'd rather use populateZonePartitionListenerWithTableProcessor or similar.

I tried to invent the new name for onTableCreate, because it is already occupied. WDYT about the "createLocalTableEntities"?

sanpwc · 2024-07-22T14:54:15Z

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

+                        var startPartsFut = new ArrayList<CompletableFuture<?>>();
+
+                        for (int i = 0; i < zoneDescriptor.partitions(); i++) {
+                            if (partitionReplicaLifecycleManager.hasLocalPartition(new ZonePartitionId(zoneDescriptor.id(), i))) {


AFAIK we've discussed possible races here and several options to solve the problem e.g. by using closure that will atomically check whether we have partition localyl and populate it tableProcessors. Am I right?

Yep, we need to replace it by the discussed methods, I made todo

ignite-3/modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java

Line 581 in 239dcbf

// TODO: https://issues.apache.org/jira/browse/IGNITE-22624 replace this method by the replicas await process.

sanpwc · 2024-07-22T14:58:14Z

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

+
+        TableImpl table =  createTableImpl(causalityToken, tableDescriptor, zoneDescriptor);
+
+        tablesVv.update(causalityToken, (ignore, e) -> inBusyLock(busyLock, () -> {


I didn't check it carefully. Do you believe that an order of updating corresponding VV's and addition of tableProcessors to the zone replica is correctly linearised?

The current guarantees is the same as in the main path, but not for storage, but for storages + table processors. So, I guess yes.

sanpwc · 2024-07-22T15:01:23Z

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

@@ -635,6 +645,133 @@ public CompletableFuture<Void> startAsync(ComponentContext componentContext) {
        });
    }

+    private CompletableFuture<Boolean> initTableFsmOnTableCreate(CreateTableEventParameters parameters) {


The most confusing method in the PR, to my opinion.

The name or in general? Let's discuss the name here #3931 (comment)

kgusakov added 30 commits June 2, 2024 22:19

fix metastore assignments

43ccf57

fix packages

6809e39

fix network messages packages

af43870

refactoring

a48e7e0

refactoring + code cleanup

6b63740

code cleanup

942b8bd

fix keys methods

fde74e1

fix checkstyle

8b11900

fix imports

dcba0c1

fix codestyle

dbaa737

remove todo

cd2075d

Merge branch 'main' into ignite-22231

992e06d

fix pr comments

501c634

fix pr comments

6f1d7d1

get the actual version of ZonePartitionId from feature branch

23ba165

rename ReplicaLifecycleManager

dd6cb7a

change ticket for TODO

8abfbf4

fix PR comment

6f4fef4

Merge branch 'main' into ignite-22231

e4f0652

fix after merge main

d77ad7c

fix multinode test

e6d9cd1

rename module

7eab822

fix package names

eaeab49

Refactor replica start/stop according to main. Added Replica interface

26ea269

fix checkstyle

5f36c6c

Merge branch 'main' into ignite-22231

4f3ca0b

working test

8ecc140

added test for two tables

6f0621f

fix compilation

16752ce

code cleanup

8124e8a

kgusakov added 7 commits June 20, 2024 23:20

Merge branch 'main' into ignite-22233

09f6129

remove old listener

37220b8

fix futures flow and assertion

cec2bff

fix checkstyle

ac4246e

Merge branch 'main' into ignite-22233

c3b3192

fix test

1a152a2

fix tests after merges

a414710

sanpwc reviewed Jun 25, 2024

View reviewed changes

kgusakov added 19 commits June 26, 2024 16:21

Merge branch 'main' into ignite-22233

3b9a9e1

fix pr comments

0af7e0f

fix pr comments

d369680

fix PR comments

7d061b2

fix PR comments

f84fe37

fix compilation

65ff65b

fix checkstyle and pmd

e813287

Merge branch 'main' into ignite-22233

a6adc7d

fix compilation

b1c5dc5

new flow

8f5b8ae

remove assignments

b756848

Merge branch 'main' into ignite-22233

b7e8024

fixed version with tx messages handling

c6411ba

Merge branch 'main' into ignite-22233

c29cdb8

remove local events

77ab50a

removed unused method

43149e6

fix redundant code

239dcbf

Merge branch 'main' into ignite-22233

63de503

fix test

1f5dbed

sanpwc reviewed Jul 22, 2024

View reviewed changes

kgusakov added 2 commits July 23, 2024 01:03

fix pr comments

c519ff9

Merge branch 'main' into ignite-22233

bd56cdc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGNITE-22233 Zone replica listener #3931

IGNITE-22233 Zone replica listener #3931

kgusakov commented Jun 16, 2024

sanpwc Jun 25, 2024

kgusakov Jun 30, 2024

sanpwc Jun 25, 2024

kgusakov Jun 30, 2024

sanpwc Jun 25, 2024

kgusakov Jun 30, 2024

sanpwc Jun 25, 2024

kgusakov Jun 26, 2024

sanpwc Jun 25, 2024

kgusakov Jun 30, 2024

sanpwc Jun 25, 2024

kgusakov Jun 26, 2024

sanpwc Jun 25, 2024

kgusakov Jun 26, 2024

sanpwc Jul 22, 2024

kgusakov Jul 22, 2024

sanpwc Jul 22, 2024

kgusakov Jul 22, 2024

sanpwc Jul 22, 2024

kgusakov Jul 22, 2024

sanpwc Jul 22, 2024

kgusakov Jul 22, 2024

sanpwc Jul 22, 2024

kgusakov Jul 22, 2024

sanpwc Jul 22, 2024

kgusakov Jul 22, 2024

sanpwc Jul 22, 2024

kgusakov Jul 22, 2024

sanpwc Jul 22, 2024

kgusakov Jul 22, 2024

sanpwc Jul 22, 2024

kgusakov Jul 22, 2024

sanpwc Jul 22, 2024

kgusakov Jul 22, 2024


		startReplica(replicaGrpId, storageIndexTracker, newReplicaListenerFut);
		CompletableFuture<ReplicaListener> newReplicaListenerFut = resultFuture.thenApply(createListener);

		@@ -410,6 +412,8 @@ public class TableManager implements IgniteTablesInternal, IgniteComponent {

		private final IndexMetaStorage indexMetaStorage;

		private PartitionReplicaLifecycleManager partitionReplicaLifecycleManager;


		assertDoesNotThrow(() -> keyValueView.put(null, 1L, 200));

		// Actually we are testing not the fair put value, but the hardcoded one from temporary noop replica listener

		@@ -37,7 +50,35 @@ public void onRead(Iterator<CommandClosure<ReadCommand>> iterator) {


		TableImpl table = createTableImpl(causalityToken, tableDescriptor, zoneDescriptor);

		tablesVv.update(causalityToken, (ignore, e) -> inBusyLock(busyLock, () -> {

IGNITE-22233 Zone replica listener #3931

Are you sure you want to change the base?

IGNITE-22233 Zone replica listener #3931

Conversation

kgusakov commented Jun 16, 2024

The Review Checklist

Notes

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

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

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

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

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

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

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