IGNITE-19227 Wait for schema availability outside JRaft threads #2450

rpuch · 2023-08-15T15:37:48Z

https://issues.apache.org/jira/browse/IGNITE-19227

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

…ommands

modules/core/src/main/java/org/apache/ignite/internal/util/VarIntUtils.java

ibessonov · 2023-08-21T08:29:55Z

...core/src/testFixtures/java/org/apache/ignite/internal/testframework/TestIgnitionManager.java


-            String delayDurationPath = "schemaSync.delayDuration";
+        configDocument = applyTestDefault(


There's no way to override this value, right?

I wish we would be able to get configuration from annotations, but that's for the future

It's marked @Immutable in the configuration, so there should not be any way to override it yet

I mean, what if subclass wants to have its own value?

If someone wants to run a cluster with a different value of some cluster-wide config property, they would use TestIgnitionManager#init() passing the config they need. The code we are looking at now is about DEFAULT values used if no explicit value was supplied when initializing the cluster.

modules/raft/src/main/java/org/apache/ignite/internal/raft/Loza.java

modules/raft/src/main/java/org/apache/ignite/internal/raft/util/OptimizedMarshaller.java

modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/ActionRequestProcessor.java

...n/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java

...rc/main/java/org/apache/ignite/internal/table/distributed/replicator/action/RequestType.java

.../org/apache/ignite/internal/table/distributed/schema/CheckCatalogVersionOnAppendEntries.java

...ava/org/apache/ignite/internal/table/distributed/schema/PartitionCommandsMarshallerImpl.java

...org/apache/ignite/internal/table/distributed/schema/PartitionCommandsMarshallerImplTest.java

modules/core/src/main/java/org/apache/ignite/internal/util/VarIntUtils.java

modules/core/src/test/java/org/apache/ignite/internal/util/VarIntUtilsTest.java

modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java

.../org/apache/ignite/internal/table/distributed/schema/CheckCatalogVersionOnAppendEntries.java

.../apache/ignite/internal/table/distributed/schema/ThreadLocalPartitionCommandsMarshaller.java

...a/org/apache/ignite/internal/table/distributed/replication/PartitionReplicaListenerTest.java

tkalkirill · 2023-08-22T11:24:28Z

...n/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java

-            finishTxCmdBldr.commitTimestampLong(commitTimestamp.longValue());
-        }
+        return catalogVersionFor(currentTimestamp)
+                .thenApply(catalogVersion -> {


Good comment from Vanya, of the same opinion.

modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java

...tegrationTest/java/org/apache/ignite/internal/schemasync/ItSchemaSyncAndReplicationTest.java

modules/runner/src/testFixtures/java/org/apache/ignite/internal/Cluster.java

tkalkirill · 2023-08-23T05:14:35Z

...n/java/org/apache/ignite/internal/table/distributed/replicator/PartitionReplicaListener.java

-            finishTxCmdBldr.commitTimestampLong(commitTimestamp.longValue());
-        }
+        return catalogVersionFor(currentTimestamp)
+                .thenApply(catalogVersion -> {


.../org/apache/ignite/internal/table/distributed/schema/CheckCatalogVersionOnAppendEntries.java

modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/ActionRequestProcessor.java

modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/IgniteRpcServer.java

ascherbakoff · 2023-08-28T10:36:07Z

modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java

        catalogManager = new CatalogManagerImpl(
                new UpdateLogImpl(metaStorageMgr),
                clockWaiter,
-                () -> schemaSyncConfig.delayDuration().value()
+                delayDurationMsSupplier


Why not pass a constant instead of closure ?

Because at the moment the constructor is called the node is not started yet, and the configuration becomes available only after we start the node

ascherbakoff · 2023-08-28T14:39:42Z

.../org/apache/ignite/internal/table/distributed/schema/CheckCatalogVersionOnAppendEntries.java

+
+            if (requiredCatalogVersion != NO_VERSION_REQUIREMENT && !isMetadataAvailableFor(requiredCatalogVersion)) {
+                LOG.warn("Metadata not yet available, group {}, required level {}.", request.groupId(), requiredCatalogVersion);
+                return RaftRpcFactory.DEFAULT //


We need to keep an eye on a number of rejected messages, some new stat field would be great. Can be done as a separate ticket.

I filed a ticket https://issues.apache.org/jira/browse/IGNITE-20298 and added a TODO mentioning this ticket

ascherbakoff · 2023-08-28T14:53:02Z

modules/raft/src/main/java/org/apache/ignite/internal/raft/server/RaftGroupOptions.java

@@ -42,6 +43,9 @@ public class RaftGroupOptions {
    /** Configuration of own striped disruptor for FSMCaller service of raft node, {@code null} means use shared disruptor. */
    private @Nullable RaftNodeDisruptorConfiguration ownFsmCallerExecutorDisruptorConfig;

+    /** Marshaller to marshall/unmarshall commands. */
+    private @Nullable Marshaller commandsMarshaller;


Why do we need to override marshaller ? For test purposes ?

For partitions, we need to quickly extract requiredCatalogVersion from command's serialized representation without parsing the whole command. To do so, I tweaked the marshaller used for partitions, but it adds one byte to each command that does not have that requiredCatalogVersion. If we use same marshaller for other (non-partition) groups, we unnecessarily increase their (serialized) sizes as well. Hence the necessity to have ability to supply a different marshaller.

rpuch force-pushed the ignite-19227 branch 2 times, most recently from e7595d9 to 25ccaf4 Compare August 17, 2023 07:54

IGNITE-19227 Wait for schema awailability out of JRaft threads

3223b05

rpuch force-pushed the ignite-19227 branch from 4d9583d to 432a76e Compare August 18, 2023 07:01

IGNITE-19227 / do not deserialize entries completely for inspection

cb4a2db

rpuch force-pushed the ignite-19227 branch from 432a76e to cb4a2db Compare August 18, 2023 10:19

rpuch added 2 commits August 18, 2023 15:40

IGNITE-19227 / add tests for partition replica listener

bafcb54

IGNITE-19227 / remove unused method

3fd307a

rpuch changed the title ~~IGNITE-19227 Wait for schema awailability out of JRaft threads~~ IGNITE-19227 Wait for schema availability out of JRaft threads Aug 18, 2023

IGNITE-19227 / test that required catalog version is sent in update c…

cfbd843

…ommands

rpuch changed the title ~~IGNITE-19227 Wait for schema availability out of JRaft threads~~ IGNITE-19227 Wait for schema availability outside JRaft threads Aug 21, 2023

ibessonov reviewed Aug 21, 2023

View reviewed changes

rpuch added 5 commits August 21, 2023 18:06

IGNITE-19227 / assert that buffer.position() is 0

adcf92c

IGNITE-19227 / extract a variable

d9149bd

IGNITE-19227 / extract a constant

72e89ca

IGNITE-19227 / rename level to version

a3aef7c

IGNITE-19227 / send -1 instead of 0 for 'no version requirement'

31a4b84

tkalkirill reviewed Aug 22, 2023

View reviewed changes

rpuch added 12 commits August 22, 2023 16:05

IGNITE-19227 / improve tests

79f94b6

IGNITE-19227 / improve javadoc

bd9a452

IGNITE-19227 / improve javadoc

f8fa5cc

IGNITE-19227 / add a @nullable

8c75cad

IGNITE-19227 / remove a linebreak

622973c

IGNITE-19227 / logging error message

c349866

IGNITE-19227 / use constant

5020bae

IGNITE-19227 / inline a variable

e61ea86

IGNITE-19227 / use constant for table name

9d6cfd3

IGNITE-19227 / introduce a constant

a738efe

IGNITE-19227 / rename a parameter

bd9ce82

IGNITE-19227 / fix a typo

ff56cde

rpuch added 4 commits August 22, 2023 18:07

IGNITE-19227 / remove a line

0d93c3e

IGNITE-19227 / use a conditional operator

abdd866

IGNITE-19227 / inline a field

3be3df8

IGNITE-19227 / move methods from RequestType

f1006c0

tkalkirill reviewed Aug 23, 2023

View reviewed changes

rpuch added 7 commits August 23, 2023 10:29

IGNITE-19227 / improve javadoc

3fca664

IGNITE-19227 / improve javadoc

d046d7c

IGNITE-19227 / move @nullable

a7bfe80

IGNITE-19227 / move classes

ed91a52

IGNITE-19227 / use future instead of a latch

e479a9d

Merge branch 'main' into ignite-19227

9f1558d

IGNITE-19227 / fix compilation

4534aec

alievmirza reviewed Aug 28, 2023

View reviewed changes

.../org/apache/ignite/internal/table/distributed/schema/CheckCatalogVersionOnAppendEntries.java Show resolved Hide resolved

ascherbakoff approved these changes Aug 28, 2023

View reviewed changes

rpuch added 7 commits August 28, 2023 20:37

Merge branch 'main' into ignite-19227

f75d7b1

IGNITE-19227 / update javadoc

e932399

IGNITE-19227 / fix imports

df803a9

IGNITE-19227 / add a TODO

7123cab

IGNITE-19227 / move nullability check to init()

91b548d

IGNITE-19227 / fix NPEs in tests

6b370de

IGNITE-19227 / disable two tests

afc3570

alievmirza approved these changes Aug 29, 2023

View reviewed changes

tkalkirill merged commit acded3f into apache:main Aug 29, 2023

tkalkirill deleted the ignite-19227 branch August 29, 2023 08:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGNITE-19227 Wait for schema availability outside JRaft threads #2450

IGNITE-19227 Wait for schema availability outside JRaft threads #2450

rpuch commented Aug 15, 2023

ibessonov Aug 21, 2023

ibessonov Aug 21, 2023

rpuch Aug 21, 2023

ibessonov Aug 23, 2023

rpuch Aug 23, 2023

tkalkirill Aug 22, 2023

tkalkirill Aug 23, 2023

ascherbakoff Aug 28, 2023

rpuch Aug 28, 2023

ascherbakoff Aug 28, 2023

rpuch Aug 28, 2023

ascherbakoff Aug 28, 2023

rpuch Aug 28, 2023


		String delayDurationPath = "schemaSync.delayDuration";
		configDocument = applyTestDefault(

IGNITE-19227 Wait for schema availability outside JRaft threads #2450

IGNITE-19227 Wait for schema availability outside JRaft threads #2450

Conversation

rpuch commented Aug 15, 2023

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