IGNITE-24702 Fix of CompactedException on the end of rebalancing #5468

denis-chudov · 2025-03-20T17:12:01Z

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

alievmirza · 2025-03-21T16:33:55Z

...rg/apache/ignite/internal/distributionzones/ItDistributionZoneMetaStorageCompactionTest.java

+        MetaStorageManager metaStorageManager = ignite.metaStorageManager();
+
+        // Wait for the rebalancing to finish.
+        assertTrue(waitForCondition(() -> {


let's check that stable has changed, something like

assertValueInStorage( metaStorageManager, stablePartAssignmentsKey(partId), (v) -> Assignments.fromBytes(v).nodes() .stream().map(Assignment::consistentId).collect(Collectors.toSet()), Set.of(node(0).name()), TIMEOUT_MILLIS );

Here the pending queue is checked for tombstone, not for emptyness, so it should work correctly

alievmirza · 2025-03-21T16:35:48Z

...ones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java

-            throw new IllegalArgumentException("causalityToken must be greater then zero [causalityToken=" + causalityToken + '"');
-        }
-
+    public CompletableFuture<Set<String>> dataNodes(HybridTimestamp timestamp, int catalogVersion, int zoneId) {


javadoc must be updated accordingly

alievmirza · 2025-03-25T10:47:40Z

...va/org/apache/ignite/internal/catalog/descriptors/CatalogHashIndexDescriptorSerializers.java

@@ -43,21 +46,22 @@ static class HashIndexDescriptorSerializerV1 implements CatalogObjectSerializer<
        public CatalogHashIndexDescriptor readFrom(CatalogObjectDataInput input) throws IOException {
            int id = input.readVarIntAsInt();
            String name = input.readUTF();
-            long updateToken = input.readVarInt();
+            long updateTimestampLong = input.readVarInt();


I do not understand, why do you change HashIndexDescriptorSerializerV1? You brake compatibility with this changes. As far as I can understand, you only need to change HashIndexDescriptorSerializerV2
But please check with sql folks regarding the correct solution.

This doesn't imply any changes in underlying storage, and caused only by the changes in the object. Now the long taken from storage is interpreted in another way: it's converted into timestamp, but this doesn't break anything.

We agreed to fix this by reading old token as initial timestamp.

alievmirza · 2025-03-25T10:51:57Z

.../java/org/apache/ignite/internal/catalog/descriptors/CatalogSchemaDescriptorSerializers.java

@@ -57,13 +60,14 @@ public CatalogSchemaDescriptor readFrom(CatalogObjectDataInput input) throws IOE

            int id = input.readVarIntAsInt();
            String name = input.readUTF();
-            long updateToken = input.readVarInt();
+            long updateTimestampLong = input.readVarInt();


The same question as for HashIndexDescriptorSerializerV2

alievmirza · 2025-03-25T10:52:22Z

.../org/apache/ignite/internal/catalog/descriptors/CatalogSortedIndexDescriptorSerializers.java

@@ -50,21 +53,22 @@ public SortedIndexDescriptorSerializerV1(CatalogEntrySerializerProvider serializ
        public CatalogSortedIndexDescriptor readFrom(CatalogObjectDataInput input) throws IOException {
            int id = input.readVarIntAsInt();
            String name = input.readUTF();
-            long updateToken = input.readVarInt();
+            long updateTimestampLong = input.readVarInt();


the same question as for HashIndexDescriptorSerializerV2

alievmirza · 2025-03-25T10:52:46Z

...a/org/apache/ignite/internal/catalog/descriptors/CatalogSystemViewDescriptorSerializers.java

@@ -53,14 +56,15 @@ public CatalogSystemViewDescriptor readFrom(CatalogObjectDataInput input) throws
            int id = input.readVarIntAsInt();
            int schemaId = input.readVarIntAsInt();
            String name = input.readUTF();
-            long updateToken = input.readVarInt();
+            long updateTimestampLong = input.readVarInt();


the same question as for HashIndexDescriptorSerializerV2

alievmirza · 2025-03-25T10:53:22Z

...n/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptorSerializers.java

@@ -49,7 +52,8 @@ public TableDescriptorSerializerV1(CatalogEntrySerializerProvider serializers) {
        public CatalogTableDescriptor readFrom(CatalogObjectDataInput input) throws IOException {
            int id = input.readVarIntAsInt();
            String name = input.readUTF();
-            long updateToken = input.readVarInt();
+            long updateTimestampLong = input.readVarInt();


the same question as for HashIndexDescriptorSerializerV2

alievmirza · 2025-03-25T10:53:38Z

...in/java/org/apache/ignite/internal/catalog/descriptors/CatalogZoneDescriptorSerializers.java

@@ -44,7 +48,8 @@ public ZoneDescriptorSerializerV1(CatalogEntrySerializerProvider serializers) {
        public CatalogZoneDescriptor readFrom(CatalogObjectDataInput input) throws IOException {
            int id = input.readVarIntAsInt();
            String name = input.readUTF();
-            long updateToken = input.readVarInt();
+            long updateTimestampLong = input.readVarInt();


the same question as for HashIndexDescriptorSerializerV2

alievmirza · 2025-03-25T10:57:12Z

...apache/ignite/internal/distributionzones/rebalance/ZoneRebalanceRaftGroupEventsListener.java

@@ -597,6 +597,9 @@ public static CompletableFuture<Void> handleReduceChanged(
        ByteArray changeTriggerKey = ZoneRebalanceUtil.pendingChangeTriggerKey(partId);
        byte[] rev = ByteUtils.longToBytesKeepingOrder(entry.revision());

+        ByteArray changeTimestampKey = ZoneRebalanceUtil.pendingChangeTriggerKey(partId);


Why did you introduce new key? can't we reuse changeTriggerKey?

But this is the one that existed before

why do you need changeTimestampKey and why do you need further changes like this

Condition changeRevisionAndTimestampDontExistOrLessThan = and( or(notExists(changeTriggerKey), value(changeTriggerKey).lt(rev)), or(notExists(changeTimestampKey), value(changeTimestampKey).lt(timestamp)) );

why can't we use changeTriggerKey and store timestamp using this key?

obsolete changes, removed, thx

ibessonov · 2025-03-25T11:31:49Z

...n/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptorSerializers.java

@@ -191,7 +195,8 @@ static class TableDescriptorSerializerV2 implements CatalogObjectSerializer<Cata
        public CatalogTableDescriptor readFrom(CatalogObjectDataInput input) throws IOException {
            int id = input.readVarIntAsInt();
            String name = input.readUTF();
-            long updateToken = input.readVarInt();
+            long updateTimestampLong = input.readVarInt();
+            HybridTimestamp updateTimestamp = updateTimestampLong == 0 ? MIN_VALUE : hybridTimestamp(updateTimestampLong);


Why is this necessary? How can it happen?

It's necessary for correct processing of 0 value that may be found in the old storage to convert it to timestamp.

ibessonov · 2025-03-25T11:32:41Z

...alog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogZoneDescriptor.java

@@ -57,7 +58,7 @@ public class CatalogZoneDescriptor extends CatalogObjectDescriptor implements Ma
     * Returns {@code true} if zone upgrade will lead to assignments recalculation.
     */
    public static boolean updateRequiresAssignmentsRecalculation(CatalogZoneDescriptor oldDescriptor, CatalogZoneDescriptor newDescriptor) {
-        if (oldDescriptor.updateToken() == newDescriptor.updateToken()) {
+        if (oldDescriptor.updateTimestamp() == newDescriptor.updateTimestamp()) {


Why is it OK to use reference equality here? This might be a bug, every reference equality check must be justified with a comment

This most likely was missed due to renaming, should be fixed.

ibessonov · 2025-03-25T11:37:55Z

...g/src/test/java/org/apache/ignite/internal/catalog/storage/TestCatalogObjectDescriptors.java

+        list.add(list.get(list.size() - 1).newDescriptor(table1.name() + "_2", 3, columns(state).subList(0, 20), hybridTimestamp(21232L),
+                "S1"));


Very unconventional formatting, please make it look better

ibessonov · 2025-03-25T11:41:51Z

...he/ignite/internal/distributionzones/rebalance/RebalanceMinimumRequiredTimeProviderImpl.java

@@ -172,9 +156,9 @@ Map<Integer, NavigableMap<Long, CatalogZoneDescriptor>> allZonesByTimestamp(
                NavigableMap<Long, CatalogZoneDescriptor> map = allZones.computeIfAbsent(zone.id(), id -> new TreeMap<>());

                if (map.isEmpty() || updateRequiresAssignmentsRecalculation(map.lastEntry().getValue(), zone)) {
-                    map.put(catalog.time(), zone);
+                    map.put(zone.updateTimestamp().longValue(), zone);


Can we use HybridTimestamp here for keys? This will make code more readable. This class is Comparable, so no worries about that. Please update all the usages of Long instead of HybridTimestamp, thank you!

alievmirza · 2025-03-26T11:57:01Z

...ution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DataNodesHistory.java

@@ -88,6 +88,11 @@ public DataNodesHistoryEntry dataNodesForTimestamp(HybridTimestamp timestamp) {
        Map.Entry<HybridTimestamp, Set<NodeWithAttributes>> entry = history.floorEntry(timestamp);

        if (entry == null) {
+            if (timestamp.equals(HybridTimestamp.MIN_VALUE)) {


let's use INITIAL_TIMESTAMP

alievmirza

I do not like solution when in dataNodes we handle INITIAL_TIMESTAMP that came from old PDS as history.firstEntry(); and return it as data nodes. From my point of view, we need to return data nodes bounded to catalog version that is passed to DistributionZoneManager#dataNodes((HybridTimestamp timestamp, int catalogVersion, int zoneId)). We could take org.apache.ignite.internal.catalog.Catalog#time and use it as timestamp for retrieving data nodes.

However, after discussion with @ibessonov, I think that we could proceed with the current solution, but with obligation to fix all compatibility issues before 3.1 release

alievmirza · 2025-03-26T12:16:45Z

.../java/org/apache/ignite/internal/catalog/descriptors/CatalogSchemaDescriptorSerializers.java


            CatalogTableDescriptor[] tables = readArray(tableDescriptorSerializer, input, CatalogTableDescriptor.class);
            CatalogIndexDescriptor[] indexes = readArray(indexSerializeHelper, input, CatalogIndexDescriptor.class);
            CatalogSystemViewDescriptor[] systemViews = readArray(viewDescriptorSerializer, input, CatalogSystemViewDescriptor.class);

-            return new CatalogSchemaDescriptor(id, name, tables, indexes, systemViews, updateToken);
+            // Here we use the initial timestamp because it's old storage. This value will be processed by data nodes manager.


This comment is correct only for ZoneDescriptor serialiser, because only ZoneDescriptor actually uses updateTimestamp and uses it in data nodes manager. For all other serialisers, like for TableDescriptor and etc, phrase like "This value will be processed by data nodes manager." is incorrect. Please, fix comments

denis-chudov added 3 commits March 20, 2025 18:28

wip

1b032f2

wip*

929bebb

Merge branch 'main' into ignite-24702

28c9676

denis-chudov force-pushed the ignite-24702 branch from e7caa72 to 28c9676 Compare March 20, 2025 17:22

denis-chudov added 4 commits March 20, 2025 19:32

fixed compilation

54dce32

fixed some tests

fe3669b

fixed some more tests

b2cb28b

one more attempts to fix checkstyle

b667303

denis-chudov changed the title ~~IGNITE-24702~~ IGNITE-24702 Fix of CompactedException on the end of rebalancing Mar 21, 2025

alievmirza suggested changes Mar 25, 2025

View reviewed changes

ibessonov reviewed Mar 25, 2025

View reviewed changes

denis-chudov added 4 commits March 25, 2025 15:26

fixed review comments

01c11bf

fixed checkstyle

cc90ece

always read old token as initial timestamp

af71726

removed obsolete changes

97ee886

alievmirza reviewed Mar 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGNITE-24702 Fix of CompactedException on the end of rebalancing #5468

IGNITE-24702 Fix of CompactedException on the end of rebalancing #5468

denis-chudov commented Mar 20, 2025

alievmirza Mar 21, 2025

denis-chudov Mar 25, 2025 •

edited

Loading

alievmirza Mar 21, 2025

denis-chudov Mar 25, 2025

alievmirza Mar 25, 2025

denis-chudov Mar 25, 2025

denis-chudov Mar 25, 2025

alievmirza Mar 25, 2025

alievmirza Mar 25, 2025

alievmirza Mar 25, 2025

alievmirza Mar 25, 2025

alievmirza Mar 25, 2025

alievmirza Mar 25, 2025

denis-chudov Mar 25, 2025

alievmirza Mar 25, 2025

alievmirza Mar 25, 2025

denis-chudov Mar 25, 2025

ibessonov Mar 25, 2025

denis-chudov Mar 25, 2025

ibessonov Mar 25, 2025

denis-chudov Mar 25, 2025

ibessonov Mar 25, 2025

denis-chudov Mar 25, 2025

ibessonov Mar 25, 2025 •

edited

Loading

denis-chudov Mar 25, 2025

alievmirza Mar 26, 2025

alievmirza left a comment •

edited

Loading

alievmirza Mar 26, 2025

		list.add(list.get(list.size() - 1).newDescriptor(table1.name() + "_2", 3, columns(state).subList(0, 20), hybridTimestamp(21232L),
		"S1"));

IGNITE-24702 Fix of CompactedException on the end of rebalancing #5468

Are you sure you want to change the base?

IGNITE-24702 Fix of CompactedException on the end of rebalancing #5468

Conversation

denis-chudov commented Mar 20, 2025

Choose a reason for hiding this comment

denis-chudov Mar 25, 2025 • 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

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

ibessonov Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alievmirza left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-chudov Mar 25, 2025 •

edited

Loading

ibessonov Mar 25, 2025 •

edited

Loading

alievmirza left a comment •

edited

Loading