[PR] IGNITE-21030 [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern opened a new pull request, #3058:
URL: https://github.com/apache/ignite-3/pull/3058

   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](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)


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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1468924759


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/serialization/CatalogSerializationUtils.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+import java.io.Serializable;
+import java.lang.reflect.Array;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.ignite.internal.util.ByteUtils;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Utility methods used for serializing catalog objects.
+ */
+public class CatalogSerializationUtils {
+    /** Reads nullable string. */
+    public static @Nullable String readNullableString(DataInput in) throws IOException {
+        if (!in.readBoolean()) {
+            return null;
+        }
+
+        return in.readUTF();
+    }
+
+    /** Writes nullable string. */
+    public static void writeNullableString(@Nullable String str, DataOutput out) throws IOException {
+        out.writeBoolean(str != null);
+
+        if (str != null) {
+            out.writeUTF(str);
+        }
+    }
+
+    /** Writes collection containing strings. */
+    public static void writeStringCollection(Collection<String> list, DataOutput out) throws IOException {
+        out.writeInt(list.size());
+
+        for (String item : list) {
+            out.writeUTF(item);
+        }
+    }
+
+    /** Reads list of strings. */
+    public static List<String> readStringList(DataInput in) throws IOException {

Review Comment:
   Done, thanks



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/serialization/UpdateLogMarshallerImpl.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+
+import com.jayway.jsonpath.internal.Utils;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataInput;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.jetbrains.annotations.TestOnly;
+
+/**
+ * Marshaller of update log entries that uses custom serializer.
+ */
+public class UpdateLogMarshallerImpl implements UpdateLogMarshaller {
+    /** Current data format version. */
+    private static final int PROTOCOL_VERSION = 1;
+
+    /** Required data format version. */
+    private final int version;
+
+    /** Serializers provider. */
+    private final CatalogEntrySerializerProvider serializers;
+
+    public UpdateLogMarshallerImpl() {
+        this.version = PROTOCOL_VERSION;
+        this.serializers = CatalogEntrySerializerProvider.DEFAULT_PROVIDER;
+    }
+
+    @TestOnly
+    UpdateLogMarshallerImpl(int version, CatalogEntrySerializerProvider serializers) {
+        this.version = version;
+        this.serializers = serializers;
+    }
+
+    @Override
+    public byte[] marshall(VersionedUpdate update) {
+        IgniteUnsafeDataOutput output = new IgniteUnsafeDataOutput(32);

Review Comment:
   Done



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1468924362


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -219,4 +221,53 @@ public void updateToken(long updateToken) {
 
         this.creationToken = this.creationToken == INITIAL_CAUSALITY_TOKEN ? updateToken : this.creationToken;
     }
+
+    /**
+     * Serializer for {@link CatalogTableDescriptor}.
+     */
+    private static class TableDescriptorSerializer implements CatalogObjectSerializer<CatalogTableDescriptor> {
+        @Override
+        public CatalogTableDescriptor readFrom(int version, IgniteDataInput input) throws IOException {
+            CatalogDescriptorBase header = CatalogObjectDescriptor.SERIALIZER.readFrom(version, input);

Review Comment:
   Removed `CatalogObjectDescriptor.SERIALIZER`.



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1467477590


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogObjectDescriptor.java:
##########
@@ -19,17 +19,22 @@
 
 import static org.apache.ignite.internal.catalog.CatalogManagerImpl.INITIAL_CAUSALITY_TOKEN;
 
-import java.io.Serializable;
+import java.io.IOException;
 import java.util.Objects;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor.CatalogDescriptorBaseSerializer.CatalogDescriptorBase;
+import org.apache.ignite.internal.catalog.serialization.CatalogObjectSerializer;
 import org.apache.ignite.internal.catalog.storage.UpdateEntry;
 import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
 
 /**
  * Base class for catalog objects.
  * TODO: IGNITE-19082 Implement custom effective serialization instead.
  */
-public abstract class CatalogObjectDescriptor implements Serializable {
-    private static final long serialVersionUID = -6525237234280004860L;
+public abstract class CatalogObjectDescriptor {
+    public static CatalogDescriptorBaseSerializer SERIALIZER = new CatalogDescriptorBaseSerializer();

Review Comment:
   Let's avoid serialization for abstract classes.



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1481331913


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogColumnCollation.java:
##########
@@ -27,6 +27,8 @@ public enum CatalogColumnCollation {
     DESC_NULLS_FIRST(false, true),
     DESC_NULLS_LAST(false, false);
 
+    private static final int ASC_FLAG_BITMASK = 1;
+    private static final int NULLS_FIRST_FLAG_BITMASK = 1 << 2;

Review Comment:
   ```suggestion
       private static final int NULLS_FIRST_FLAG_BITMASK = 1 << 1;
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1469640591


##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/serialization/CatalogEntrySerializationCompatibilityTest.java:
##########
@@ -0,0 +1,429 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrows;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Objects;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests to verify catalog storage entries serialization compatibility.
+ */
+public class CatalogEntrySerializationCompatibilityTest {
+    private final CatalogEntrySerializerProvider serializerProvider = (typeId) -> {
+        switch (typeId) {
+            case 0:
+                return TestUpdateEntrySerializerV1.INSTANCE;
+
+            case 1:
+                return TestUpdateEntrySerializerV2.INSTANCE;
+
+            case 2:
+                return TestUpdateEntrySerializerV3.INSTANCE;
+
+            default:
+                throw new IllegalArgumentException("Unexpected type: " + typeId);
+        }
+    };
+
+    /**
+     * Checks whether the old version of an entity can be read by the new version's serializer.
+     */
+    @Test
+    public void readPreviousVersionEntry() {
+        // Writes a version 1 entry, reads it using the version 2 and 3 serializer.
+        {
+            TestUpdateEntry entryV1 = TestEntryFactory.create(1);
+
+            UpdateLogMarshaller marshallerV1 = new UpdateLogMarshallerImpl(1, serializerProvider);
+
+            List<UpdateEntry> entries = List.of(entryV1, entryV1);
+
+            VersionedUpdate update = new VersionedUpdate(1, 2, entries);
+
+            byte[] bytesV1 = marshallerV1.marshall(update);
+
+            VersionedUpdate deserialized = marshallerV1.unmarshall(bytesV1);
+            assertThat(deserialized.entries(), equalTo(entries));
+
+            UpdateLogMarshaller marshallerV2 = new UpdateLogMarshallerImpl(2, serializerProvider);
+            deserialized = marshallerV2.unmarshall(bytesV1);
+            assertThat(deserialized.entries(), equalTo(entries));
+
+            UpdateLogMarshaller marshallerV3 = new UpdateLogMarshallerImpl(3, serializerProvider);
+            deserialized = marshallerV3.unmarshall(bytesV1);
+            assertThat(deserialized.entries(), equalTo(entries));
+        }
+
+        // Writes a version 2 entry, reads it using the version 3 serializer.
+        {
+            TestUpdateEntryV2 entryV2 = TestEntryFactory.create(2);
+
+            UpdateLogMarshaller marshallerV2 = new UpdateLogMarshallerImpl(2, serializerProvider);
+
+            List<UpdateEntry> entries = List.of(entryV2, entryV2);
+            VersionedUpdate update = new VersionedUpdate(1, 2, entries);
+
+            byte[] bytesV2 = marshallerV2.marshall(update);
+
+            VersionedUpdate deserialized = marshallerV2.unmarshall(bytesV2);
+            assertThat(deserialized.entries(), equalTo(entries));
+
+            UpdateLogMarshaller marshallerV3 = new UpdateLogMarshallerImpl(3, serializerProvider);
+            deserialized = marshallerV3.unmarshall(bytesV2);
+            assertThat(deserialized.entries(), equalTo(entries));
+        }
+    }
+
+    /**
+     * Checks whether the entry can be written in the previous format.
+     */
+    @Test
+    public void writePreviousVersionEntry() {

Review Comment:
   The purpose of the test is to check whether, after changing the protocol version, we can work on the old version. We need to be able to write entries in the old format until the entire cluster can use the new format.



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1468924794


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/serialization/UpdateLogMarshallerImpl.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+
+import com.jayway.jsonpath.internal.Utils;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataInput;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.jetbrains.annotations.TestOnly;
+
+/**
+ * Marshaller of update log entries that uses custom serializer.
+ */
+public class UpdateLogMarshallerImpl implements UpdateLogMarshaller {
+    /** Current data format version. */
+    private static final int PROTOCOL_VERSION = 1;
+
+    /** Required data format version. */
+    private final int version;
+
+    /** Serializers provider. */
+    private final CatalogEntrySerializerProvider serializers;
+
+    public UpdateLogMarshallerImpl() {
+        this.version = PROTOCOL_VERSION;
+        this.serializers = CatalogEntrySerializerProvider.DEFAULT_PROVIDER;
+    }
+
+    @TestOnly
+    UpdateLogMarshallerImpl(int version, CatalogEntrySerializerProvider serializers) {
+        this.version = version;
+        this.serializers = serializers;
+    }
+
+    @Override
+    public byte[] marshall(VersionedUpdate update) {
+        IgniteUnsafeDataOutput output = new IgniteUnsafeDataOutput(32);
+
+        try {
+            output.writeShort(version);
+
+            output.writeInt(update.entries().size());
+
+            for (UpdateEntry entry : update.entries()) {
+                output.writeShort(entry.typeId());
+
+                CatalogObjectSerializer<? super UpdateEntry> serializer = serializers.get(entry.typeId());
+                serializer.writeTo(entry, version, output);
+            }
+
+            output.writeInt(update.version());
+            output.writeLong(update.delayDurationMs());
+

Review Comment:
   Done



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/serialization/UpdateLogMarshallerImpl.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+
+import com.jayway.jsonpath.internal.Utils;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataInput;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.jetbrains.annotations.TestOnly;
+
+/**
+ * Marshaller of update log entries that uses custom serializer.
+ */
+public class UpdateLogMarshallerImpl implements UpdateLogMarshaller {
+    /** Current data format version. */
+    private static final int PROTOCOL_VERSION = 1;
+
+    /** Required data format version. */
+    private final int version;

Review Comment:
   Done



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1467476740


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -219,4 +221,53 @@ public void updateToken(long updateToken) {
 
         this.creationToken = this.creationToken == INITIAL_CAUSALITY_TOKEN ? updateToken : this.creationToken;
     }
+
+    /**
+     * Serializer for {@link CatalogTableDescriptor}.
+     */
+    private static class TableDescriptorSerializer implements CatalogObjectSerializer<CatalogTableDescriptor> {
+        @Override
+        public CatalogTableDescriptor readFrom(int version, IgniteDataInput input) throws IOException {
+            CatalogDescriptorBase header = CatalogObjectDescriptor.SERIALIZER.readFrom(version, input);
+            CatalogTableSchemaVersions schemaVersions = CatalogTableSchemaVersions.SERIALIZER.readFrom(version, input);
+            List<CatalogTableColumnDescriptor> columns = readList(version, CatalogTableColumnDescriptor.SERIALIZER, input);
+
+            int schemaId = input.readInt();
+            int pkIndexId = input.readInt();
+            int zoneId = input.readInt();
+
+            List<String> primaryKeyColumns = readStringList(input);
+            List<String> colocationColumns = readStringList(input);
+
+            long creationToken = input.readLong();
+
+            return new CatalogTableDescriptor(
+                    header.id(),
+                    schemaId,
+                    pkIndexId,
+                    header.name(),
+                    zoneId,
+                    columns,
+                    primaryKeyColumns,
+                    colocationColumns,
+                    schemaVersions,
+                    header.updateToken(),
+                    creationToken
+            );
+        }
+
+        @Override
+        public void writeTo(CatalogTableDescriptor descriptor, int version, IgniteDataOutput output) throws IOException {
+            CatalogObjectDescriptor.SERIALIZER.writeTo(new CatalogDescriptorBase(descriptor), version, output);
+            CatalogTableSchemaVersions.SERIALIZER.writeTo(descriptor.schemaVersions(), version, output);
+            writeList(descriptor.columns(), version, CatalogTableColumnDescriptor.SERIALIZER, output);
+
+            output.writeInt(descriptor.schemaId());
+            output.writeInt(descriptor.primaryKeyIndexId());
+            output.writeInt(descriptor.zoneId());
+            writeStringCollection(descriptor.primaryKeyColumns(), output);
+            writeStringCollection(descriptor.colocationColumns(), output);

Review Comment:
   We can skip colocation columns if they match key columns.
   I guess we pass the same collection (the same object) to constructor in that case.



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1467504556


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/serialization/UpdateLogMarshallerImpl.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+
+import com.jayway.jsonpath.internal.Utils;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataInput;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.jetbrains.annotations.TestOnly;
+
+/**
+ * Marshaller of update log entries that uses custom serializer.
+ */
+public class UpdateLogMarshallerImpl implements UpdateLogMarshaller {
+    /** Current data format version. */
+    private static final int PROTOCOL_VERSION = 1;
+
+    /** Required data format version. */
+    private final int version;

Review Comment:
   ```suggestion
       private final int protocolVersion;
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1468924712


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/serialization/CatalogSerializationUtils.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+import java.io.Serializable;
+import java.lang.reflect.Array;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.ignite.internal.util.ByteUtils;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Utility methods used for serializing catalog objects.
+ */
+public class CatalogSerializationUtils {
+    /** Reads nullable string. */
+    public static @Nullable String readNullableString(DataInput in) throws IOException {
+        if (!in.readBoolean()) {
+            return null;
+        }
+
+        return in.readUTF();
+    }
+
+    /** Writes nullable string. */
+    public static void writeNullableString(@Nullable String str, DataOutput out) throws IOException {
+        out.writeBoolean(str != null);
+
+        if (str != null) {
+            out.writeUTF(str);
+        }
+    }
+
+    /** Writes collection containing strings. */
+    public static void writeStringCollection(Collection<String> list, DataOutput out) throws IOException {
+        out.writeInt(list.size());
+
+        for (String item : list) {
+            out.writeUTF(item);
+        }
+    }
+
+    /** Reads list of strings. */
+    public static List<String> readStringList(DataInput in) throws IOException {
+        int size = in.readInt();
+
+        return readStringCollection(in, size, new ArrayList<>(size));
+    }
+
+    /** Reads set of strings. */
+    public static Set<String> readStringSet(DataInput in) throws IOException {
+        int size = in.readInt();
+
+        return readStringCollection(in, size, new HashSet<>());

Review Comment:
   didn't find this method, replaced with `new HashSet<>(capacity(size));`



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/serialization/CatalogSerializationUtils.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+import java.io.Serializable;
+import java.lang.reflect.Array;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.ignite.internal.util.ByteUtils;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Utility methods used for serializing catalog objects.
+ */
+public class CatalogSerializationUtils {
+    /** Reads nullable string. */
+    public static @Nullable String readNullableString(DataInput in) throws IOException {
+        if (!in.readBoolean()) {
+            return null;
+        }
+
+        return in.readUTF();
+    }
+
+    /** Writes nullable string. */
+    public static void writeNullableString(@Nullable String str, DataOutput out) throws IOException {
+        out.writeBoolean(str != null);
+
+        if (str != null) {
+            out.writeUTF(str);
+        }
+    }
+
+    /** Writes collection containing strings. */
+    public static void writeStringCollection(Collection<String> list, DataOutput out) throws IOException {
+        out.writeInt(list.size());
+
+        for (String item : list) {
+            out.writeUTF(item);
+        }
+    }
+
+    /** Reads list of strings. */
+    public static List<String> readStringList(DataInput in) throws IOException {
+        int size = in.readInt();
+
+        return readStringCollection(in, size, new ArrayList<>(size));
+    }
+
+    /** Reads set of strings. */
+    public static Set<String> readStringSet(DataInput in) throws IOException {
+        int size = in.readInt();
+
+        return readStringCollection(in, size, new HashSet<>());

Review Comment:
   didn't find such method, replaced with `new HashSet<>(capacity(size));`



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1467482237


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/serialization/CatalogSerializationUtils.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+import java.io.Serializable;
+import java.lang.reflect.Array;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.ignite.internal.util.ByteUtils;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Utility methods used for serializing catalog objects.
+ */
+public class CatalogSerializationUtils {
+    /** Reads nullable string. */
+    public static @Nullable String readNullableString(DataInput in) throws IOException {
+        if (!in.readBoolean()) {
+            return null;
+        }
+
+        return in.readUTF();
+    }
+
+    /** Writes nullable string. */
+    public static void writeNullableString(@Nullable String str, DataOutput out) throws IOException {
+        out.writeBoolean(str != null);
+
+        if (str != null) {
+            out.writeUTF(str);
+        }
+    }
+
+    /** Writes collection containing strings. */
+    public static void writeStringCollection(Collection<String> list, DataOutput out) throws IOException {
+        out.writeInt(list.size());
+
+        for (String item : list) {
+            out.writeUTF(item);
+        }
+    }
+
+    /** Reads list of strings. */
+    public static List<String> readStringList(DataInput in) throws IOException {
+        int size = in.readInt();
+
+        return readStringCollection(in, size, new ArrayList<>(size));
+    }
+
+    /** Reads set of strings. */
+    public static Set<String> readStringSet(DataInput in) throws IOException {
+        int size = in.readInt();
+
+        return readStringCollection(in, size, new HashSet<>());

Review Comment:
   ```suggestion
           return readStringCollection(in, size, IgniteUtils.newHashSet(size));
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1479642057


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/serialization/UpdateLogMarshallerImpl.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.storage.serialization;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+
+import org.apache.ignite.internal.catalog.storage.SnapshotEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateLogEvent;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataInput;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.jetbrains.annotations.TestOnly;
+
+/**
+ * Marshaller of update log entries that uses custom serializer.
+ *
+ *<p>External serialization was implemented for two reasons:
+ *<ul>
+ * <li>Minimize the size of the serialized update log entry.</li>
+ * <li>Backward compatibility support when changing existing or adding new entry.</li>
+ * </ul>
+ * At the moment, the serialization format is specified manually.
+ *
+ *<p>While backward compatibility is not required, entities can change arbitrarily without changing the data format version.
+ * However, every change to an entry's fields requires a change to the serializer that serializes that entry.
+ *
+ *<p>Basic format description:
+ *<pre>
+ * (size) | description
+ * ------------------------------
+ *     2  | data format version ({@link #PROTOCOL_VERSION})
+ *     2  | entry type ({@link VersionedUpdate} or {@link SnapshotEntry})
+ *
+ * [fields of the corresponding event follow (for example, {@link VersionedUpdate}]
+ *
+ *     4  | version
+ *     8  | delayDurationMs
+ *
+ * [list of entries follow]
+ *
+ *     4  | entries size
+ *     2  | entry type (see {@link MarshallableEntryType#id()})
+ *
+ * [entry fields follow]

Review Comment:
   ```suggestion
    * <list> | list of {@link MarshallableEntry}
    * </pre>
    *
    * MarshallableEntry format:
    * <pre>
    *     4  | entries size
    *     2  | entry type (see {@link MarshallableEntryType#id()})
    *       | other entry fields.
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1467503963


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/serialization/UpdateLogMarshallerImpl.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+
+import com.jayway.jsonpath.internal.Utils;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataInput;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.jetbrains.annotations.TestOnly;
+
+/**
+ * Marshaller of update log entries that uses custom serializer.
+ */
+public class UpdateLogMarshallerImpl implements UpdateLogMarshaller {
+    /** Current data format version. */
+    private static final int PROTOCOL_VERSION = 1;
+
+    /** Required data format version. */
+    private final int version;
+
+    /** Serializers provider. */
+    private final CatalogEntrySerializerProvider serializers;
+
+    public UpdateLogMarshallerImpl() {
+        this.version = PROTOCOL_VERSION;
+        this.serializers = CatalogEntrySerializerProvider.DEFAULT_PROVIDER;
+    }
+
+    @TestOnly
+    UpdateLogMarshallerImpl(int version, CatalogEntrySerializerProvider serializers) {
+        this.version = version;
+        this.serializers = serializers;
+    }
+
+    @Override
+    public byte[] marshall(VersionedUpdate update) {
+        IgniteUnsafeDataOutput output = new IgniteUnsafeDataOutput(32);
+
+        try {
+            output.writeShort(version);
+
+            output.writeInt(update.entries().size());
+
+            for (UpdateEntry entry : update.entries()) {
+                output.writeShort(entry.typeId());
+
+                CatalogObjectSerializer<? super UpdateEntry> serializer = serializers.get(entry.typeId());
+                serializer.writeTo(entry, version, output);
+            }
+
+            output.writeInt(update.version());
+            output.writeLong(update.delayDurationMs());
+

Review Comment:
   ```suggestion
               output.writeInt(update.version());
               output.writeLong(update.delayDurationMs());
              
               output.writeInt(update.entries().size());
               for (UpdateEntry entry : update.entries()) {
                   output.writeShort(entry.typeId());
   
                   CatalogObjectSerializer<? super UpdateEntry> serializer = serializers.get(entry.typeId());
                   serializer.writeTo(entry, version, output);
               }
   
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1468924362


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -219,4 +221,53 @@ public void updateToken(long updateToken) {
 
         this.creationToken = this.creationToken == INITIAL_CAUSALITY_TOKEN ? updateToken : this.creationToken;
     }
+
+    /**
+     * Serializer for {@link CatalogTableDescriptor}.
+     */
+    private static class TableDescriptorSerializer implements CatalogObjectSerializer<CatalogTableDescriptor> {
+        @Override
+        public CatalogTableDescriptor readFrom(int version, IgniteDataInput input) throws IOException {
+            CatalogDescriptorBase header = CatalogObjectDescriptor.SERIALIZER.readFrom(version, input);

Review Comment:
   Removed atalogObjectDescriptor.SERIALIZER.



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -219,4 +221,53 @@ public void updateToken(long updateToken) {
 
         this.creationToken = this.creationToken == INITIAL_CAUSALITY_TOKEN ? updateToken : this.creationToken;
     }
+
+    /**
+     * Serializer for {@link CatalogTableDescriptor}.
+     */
+    private static class TableDescriptorSerializer implements CatalogObjectSerializer<CatalogTableDescriptor> {
+        @Override
+        public CatalogTableDescriptor readFrom(int version, IgniteDataInput input) throws IOException {
+            CatalogDescriptorBase header = CatalogObjectDescriptor.SERIALIZER.readFrom(version, input);
+            CatalogTableSchemaVersions schemaVersions = CatalogTableSchemaVersions.SERIALIZER.readFrom(version, input);
+            List<CatalogTableColumnDescriptor> columns = readList(version, CatalogTableColumnDescriptor.SERIALIZER, input);
+
+            int schemaId = input.readInt();
+            int pkIndexId = input.readInt();
+            int zoneId = input.readInt();
+
+            List<String> primaryKeyColumns = readStringList(input);
+            List<String> colocationColumns = readStringList(input);
+
+            long creationToken = input.readLong();
+
+            return new CatalogTableDescriptor(
+                    header.id(),
+                    schemaId,
+                    pkIndexId,
+                    header.name(),
+                    zoneId,
+                    columns,
+                    primaryKeyColumns,
+                    colocationColumns,
+                    schemaVersions,
+                    header.updateToken(),
+                    creationToken
+            );
+        }
+
+        @Override
+        public void writeTo(CatalogTableDescriptor descriptor, int version, IgniteDataOutput output) throws IOException {
+            CatalogObjectDescriptor.SERIALIZER.writeTo(new CatalogDescriptorBase(descriptor), version, output);

Review Comment:
   Done



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1479644576


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogColumnCollation.java:
##########
@@ -27,6 +27,8 @@ public enum CatalogColumnCollation {
     DESC_NULLS_FIRST(false, true),
     DESC_NULLS_LAST(false, false);
 
+    private static final int ASC_BIT_POS = 0;
+    private static final int NULLS_FIRST_BIT_POS = 1;

Review Comment:
   ```suggestion
       private static final int ASC_BIT_POS = 1;
       private static final int NULLS_FIRST_BIT_POS = 1 << 1;
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1479655902


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/serialization/UpdateLogMarshallerImpl.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.storage.serialization;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+
+import org.apache.ignite.internal.catalog.storage.SnapshotEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateLogEvent;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataInput;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.jetbrains.annotations.TestOnly;
+
+/**
+ * Marshaller of update log entries that uses custom serializer.
+ *
+ *<p>External serialization was implemented for two reasons:
+ *<ul>
+ * <li>Minimize the size of the serialized update log entry.</li>
+ * <li>Backward compatibility support when changing existing or adding new entry.</li>
+ * </ul>
+ * At the moment, the serialization format is specified manually.
+ *
+ *<p>While backward compatibility is not required, entities can change arbitrarily without changing the data format version.
+ * However, every change to an entry's fields requires a change to the serializer that serializes that entry.
+ *
+ *<p>Basic format description:
+ *<pre>
+ * (size) | description
+ * ------------------------------
+ *     2  | data format version ({@link #PROTOCOL_VERSION})
+ *     2  | entry type ({@link VersionedUpdate} or {@link SnapshotEntry})
+ *
+ * [fields of the corresponding event follow (for example, {@link VersionedUpdate}]
+ *
+ *     4  | version
+ *     8  | delayDurationMs
+ *
+ * [list of entries follow]
+ *
+ *     4  | entries size
+ *     2  | entry type (see {@link MarshallableEntryType#id()})
+ *
+ * [entry fields follow]

Review Comment:
   Done thanks.



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1467523449


##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/serialization/CatalogEntrySerializationCompatibilityTest.java:
##########
@@ -0,0 +1,371 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrows;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Objects;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests to verify catalog storage entries serialization compatibility.
+ */
+public class CatalogEntrySerializationCompatibilityTest {
+    private final CatalogEntrySerializerProvider serializerProvider = (ignore) -> TestUpdateEntrySerializer.INSTANCE;

Review Comment:
   You have a single serializer version here.
   I think the test idea is to have at least 2 versions, "old" and "new" ones. Where "old" one supports version 1 only, and "new" one supports version 1 and 2.



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1479594701


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogColumnCollation.java:
##########
@@ -70,4 +70,17 @@ public boolean asc() {
     public boolean nullsFirst() {
         return nullsFirst;
     }
+
+    /** Writes collation to byte. */
+    static byte pack(CatalogColumnCollation collation) {
+        return  (byte) ((collation.asc() ? 1 : 0) | (collation.nullsFirst() ? 2 : 0));
+    }
+
+    /** Reads collation from byte. */
+    static CatalogColumnCollation unpack(byte collationBits) {
+        boolean asc = (collationBits & 1) == 1;
+        boolean nullsFirst = (collationBits & 2) == 2;

Review Comment:
   Added constants for bit positions.



##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogIndexDescriptor.java:
##########
@@ -66,8 +67,40 @@ public int creationCatalogVersion() {
         return creationCatalogVersion;
     }
 
+    /** Returns catalog index descriptor type. */
+    public CatalogIndexDescriptorType indexType() {
+        return indexType;
+    }
+
     @Override
     public String toString() {
         return S.toString(CatalogIndexDescriptor.class, this, super.toString());
     }
+
+    /** Catalog index descriptor type. */
+    public enum CatalogIndexDescriptorType {
+        HASH(0),
+        SORTED(1);
+
+        private final int id;

Review Comment:
   Done.



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1479638321


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/serialization/UpdateLogMarshallerImpl.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.storage.serialization;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+
+import org.apache.ignite.internal.catalog.storage.SnapshotEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateLogEvent;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataInput;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.jetbrains.annotations.TestOnly;
+
+/**
+ * Marshaller of update log entries that uses custom serializer.
+ *
+ *<p>External serialization was implemented for two reasons:
+ *<ul>
+ * <li>Minimize the size of the serialized update log entry.</li>
+ * <li>Backward compatibility support when changing existing or adding new entry.</li>
+ * </ul>
+ * At the moment, the serialization format is specified manually.
+ *
+ *<p>While backward compatibility is not required, entities can change arbitrarily without changing the data format version.
+ * However, every change to an entry's fields requires a change to the serializer that serializes that entry.
+ *
+ *<p>Basic format description:
+ *<pre>
+ * (size) | description
+ * ------------------------------
+ *     2  | data format version ({@link #PROTOCOL_VERSION})
+ *     2  | entry type ({@link VersionedUpdate} or {@link SnapshotEntry})
+ *
+ * [fields of the corresponding event follow (for example, {@link VersionedUpdate}]
+ *

Review Comment:
   ```suggestion
    *     2  | entry type ({@link VersionedUpdate} or {@link SnapshotEntry})
    * <list> | fields of the corresponding event follow (for example, {@link VersionedUpdate} or {@SnapshotEntry})
    * </pre>
    * 
    * Versioned Update format description:
    * <pre>
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1479445451


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogIndexDescriptor.java:
##########
@@ -66,8 +67,40 @@ public int creationCatalogVersion() {
         return creationCatalogVersion;
     }
 
+    /** Returns catalog index descriptor type. */
+    public CatalogIndexDescriptorType indexType() {
+        return indexType;
+    }
+
     @Override
     public String toString() {
         return S.toString(CatalogIndexDescriptor.class, this, super.toString());
     }
+
+    /** Catalog index descriptor type. */
+    public enum CatalogIndexDescriptorType {
+        HASH(0),
+        SORTED(1);
+
+        private final int id;

Review Comment:
   ```suggestion
           private final int typeId;
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1469143977


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -219,4 +221,53 @@ public void updateToken(long updateToken) {
 
         this.creationToken = this.creationToken == INITIAL_CAUSALITY_TOKEN ? updateToken : this.creationToken;
     }
+
+    /**
+     * Serializer for {@link CatalogTableDescriptor}.
+     */
+    private static class TableDescriptorSerializer implements CatalogObjectSerializer<CatalogTableDescriptor> {
+        @Override
+        public CatalogTableDescriptor readFrom(int version, IgniteDataInput input) throws IOException {
+            CatalogDescriptorBase header = CatalogObjectDescriptor.SERIALIZER.readFrom(version, input);
+            CatalogTableSchemaVersions schemaVersions = CatalogTableSchemaVersions.SERIALIZER.readFrom(version, input);
+            List<CatalogTableColumnDescriptor> columns = readList(version, CatalogTableColumnDescriptor.SERIALIZER, input);
+
+            int schemaId = input.readInt();
+            int pkIndexId = input.readInt();
+            int zoneId = input.readInt();
+
+            List<String> primaryKeyColumns = readStringList(input);
+            List<String> colocationColumns = readStringList(input);
+
+            long creationToken = input.readLong();
+
+            return new CatalogTableDescriptor(
+                    header.id(),
+                    schemaId,
+                    pkIndexId,
+                    header.name(),
+                    zoneId,
+                    columns,
+                    primaryKeyColumns,
+                    colocationColumns,
+                    schemaVersions,
+                    header.updateToken(),
+                    creationToken
+            );
+        }
+
+        @Override
+        public void writeTo(CatalogTableDescriptor descriptor, int version, IgniteDataOutput output) throws IOException {
+            CatalogObjectDescriptor.SERIALIZER.writeTo(new CatalogDescriptorBase(descriptor), version, output);
+            CatalogTableSchemaVersions.SERIALIZER.writeTo(descriptor.schemaVersions(), version, output);
+            writeList(descriptor.columns(), version, CatalogTableColumnDescriptor.SERIALIZER, output);
+
+            output.writeInt(descriptor.schemaId());
+            output.writeInt(descriptor.primaryKeyIndexId());
+            output.writeInt(descriptor.zoneId());
+            writeStringCollection(descriptor.primaryKeyColumns(), output);
+            writeStringCollection(descriptor.colocationColumns(), output);

Review Comment:
   I'm not sure it's worth adding such optimization. As I understand we need to compare collections for equality here + store special marker (for example size = `-2`) that collection are equal. :thinking: 



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1467496511


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/serialization/UpdateLogMarshallerImpl.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+
+import com.jayway.jsonpath.internal.Utils;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataInput;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.jetbrains.annotations.TestOnly;
+
+/**
+ * Marshaller of update log entries that uses custom serializer.
+ */
+public class UpdateLogMarshallerImpl implements UpdateLogMarshaller {
+    /** Current data format version. */
+    private static final int PROTOCOL_VERSION = 1;
+
+    /** Required data format version. */
+    private final int version;
+
+    /** Serializers provider. */
+    private final CatalogEntrySerializerProvider serializers;
+
+    public UpdateLogMarshallerImpl() {
+        this.version = PROTOCOL_VERSION;
+        this.serializers = CatalogEntrySerializerProvider.DEFAULT_PROVIDER;
+    }
+
+    @TestOnly
+    UpdateLogMarshallerImpl(int version, CatalogEntrySerializerProvider serializers) {
+        this.version = version;
+        this.serializers = serializers;
+    }
+
+    @Override
+    public byte[] marshall(VersionedUpdate update) {
+        IgniteUnsafeDataOutput output = new IgniteUnsafeDataOutput(32);

Review Comment:
   ```suggestion
           IgniteUnsafeDataOutput output = new IgniteUnsafeDataOutput(256);
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1479656669


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogColumnCollation.java:
##########
@@ -27,6 +27,8 @@ public enum CatalogColumnCollation {
     DESC_NULLS_FIRST(false, true),
     DESC_NULLS_LAST(false, false);
 
+    private static final int ASC_BIT_POS = 0;
+    private static final int NULLS_FIRST_BIT_POS = 1;

Review Comment:
   :thinking: :thinking:  reverted last changes and renamed to ASC_FLAG_**BITMASK**...



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1479442662


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogColumnCollation.java:
##########
@@ -70,4 +70,17 @@ public boolean asc() {
     public boolean nullsFirst() {
         return nullsFirst;
     }
+
+    /** Writes collation to byte. */
+    static byte pack(CatalogColumnCollation collation) {
+        return  (byte) ((collation.asc() ? 1 : 0) | (collation.nullsFirst() ? 2 : 0));
+    }
+
+    /** Reads collation from byte. */
+    static CatalogColumnCollation unpack(byte collationBits) {
+        boolean asc = (collationBits & 1) == 1;
+        boolean nullsFirst = (collationBits & 2) == 2;

Review Comment:
   Let's convert magic numbers to constants.



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern merged PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058


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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1481337111


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogColumnCollation.java:
##########
@@ -27,6 +27,8 @@ public enum CatalogColumnCollation {
     DESC_NULLS_FIRST(false, true),
     DESC_NULLS_LAST(false, false);
 
+    private static final int ASC_FLAG_BITMASK = 1;
+    private static final int NULLS_FIRST_FLAG_BITMASK = 1 << 2;

Review Comment:
   Thanks, missed it.



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1467496511


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/serialization/UpdateLogMarshallerImpl.java:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+
+import com.jayway.jsonpath.internal.Utils;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataInput;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.jetbrains.annotations.TestOnly;
+
+/**
+ * Marshaller of update log entries that uses custom serializer.
+ */
+public class UpdateLogMarshallerImpl implements UpdateLogMarshaller {
+    /** Current data format version. */
+    private static final int PROTOCOL_VERSION = 1;
+
+    /** Required data format version. */
+    private final int version;
+
+    /** Serializers provider. */
+    private final CatalogEntrySerializerProvider serializers;
+
+    public UpdateLogMarshallerImpl() {
+        this.version = PROTOCOL_VERSION;
+        this.serializers = CatalogEntrySerializerProvider.DEFAULT_PROVIDER;
+    }
+
+    @TestOnly
+    UpdateLogMarshallerImpl(int version, CatalogEntrySerializerProvider serializers) {
+        this.version = version;
+        this.serializers = serializers;
+    }
+
+    @Override
+    public byte[] marshall(VersionedUpdate update) {
+        IgniteUnsafeDataOutput output = new IgniteUnsafeDataOutput(32);

Review Comment:
   ```suggestion
           IgniteUnsafeDataOutput output = new IgniteUnsafeDataOutput(128);
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1467523449


##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/serialization/CatalogEntrySerializationCompatibilityTest.java:
##########
@@ -0,0 +1,371 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrows;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Objects;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests to verify catalog storage entries serialization compatibility.
+ */
+public class CatalogEntrySerializationCompatibilityTest {
+    private final CatalogEntrySerializerProvider serializerProvider = (ignore) -> TestUpdateEntrySerializer.INSTANCE;

Review Comment:
   You have a single serializer version here.
   I think the idea of test is having at least 2 serializers, "old" and "new" ones. Where "old" one supports version 1 only, and "new" one supports version 1 and 2.



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1469389829


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -219,4 +221,53 @@ public void updateToken(long updateToken) {
 
         this.creationToken = this.creationToken == INITIAL_CAUSALITY_TOKEN ? updateToken : this.creationToken;
     }
+
+    /**
+     * Serializer for {@link CatalogTableDescriptor}.
+     */
+    private static class TableDescriptorSerializer implements CatalogObjectSerializer<CatalogTableDescriptor> {
+        @Override
+        public CatalogTableDescriptor readFrom(int version, IgniteDataInput input) throws IOException {
+            CatalogDescriptorBase header = CatalogObjectDescriptor.SERIALIZER.readFrom(version, input);
+            CatalogTableSchemaVersions schemaVersions = CatalogTableSchemaVersions.SERIALIZER.readFrom(version, input);
+            List<CatalogTableColumnDescriptor> columns = readList(version, CatalogTableColumnDescriptor.SERIALIZER, input);
+
+            int schemaId = input.readInt();
+            int pkIndexId = input.readInt();
+            int zoneId = input.readInt();
+
+            List<String> primaryKeyColumns = readStringList(input);
+            List<String> colocationColumns = readStringList(input);
+
+            long creationToken = input.readLong();
+
+            return new CatalogTableDescriptor(
+                    header.id(),
+                    schemaId,
+                    pkIndexId,
+                    header.name(),
+                    zoneId,
+                    columns,
+                    primaryKeyColumns,
+                    colocationColumns,
+                    schemaVersions,
+                    header.updateToken(),
+                    creationToken
+            );
+        }
+
+        @Override
+        public void writeTo(CatalogTableDescriptor descriptor, int version, IgniteDataOutput output) throws IOException {
+            CatalogObjectDescriptor.SERIALIZER.writeTo(new CatalogDescriptorBase(descriptor), version, output);
+            CatalogTableSchemaVersions.SERIALIZER.writeTo(descriptor.schemaVersions(), version, output);
+            writeList(descriptor.columns(), version, CatalogTableColumnDescriptor.SERIALIZER, output);
+
+            output.writeInt(descriptor.schemaId());
+            output.writeInt(descriptor.primaryKeyIndexId());
+            output.writeInt(descriptor.zoneId());
+            writeStringCollection(descriptor.primaryKeyColumns(), output);
+            writeStringCollection(descriptor.colocationColumns(), output);

Review Comment:
   I think we can compare them by reference.



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1468924586


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogObjectDescriptor.java:
##########
@@ -19,17 +19,22 @@
 
 import static org.apache.ignite.internal.catalog.CatalogManagerImpl.INITIAL_CAUSALITY_TOKEN;
 
-import java.io.Serializable;
+import java.io.IOException;
 import java.util.Objects;
+import org.apache.ignite.internal.catalog.descriptors.CatalogObjectDescriptor.CatalogDescriptorBaseSerializer.CatalogDescriptorBase;
+import org.apache.ignite.internal.catalog.serialization.CatalogObjectSerializer;
 import org.apache.ignite.internal.catalog.storage.UpdateEntry;
 import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
 
 /**
  * Base class for catalog objects.
  * TODO: IGNITE-19082 Implement custom effective serialization instead.
  */
-public abstract class CatalogObjectDescriptor implements Serializable {
-    private static final long serialVersionUID = -6525237234280004860L;
+public abstract class CatalogObjectDescriptor {
+    public static CatalogDescriptorBaseSerializer SERIALIZER = new CatalogDescriptorBaseSerializer();

Review Comment:
   Done



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1467472751


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -219,4 +221,53 @@ public void updateToken(long updateToken) {
 
         this.creationToken = this.creationToken == INITIAL_CAUSALITY_TOKEN ? updateToken : this.creationToken;
     }
+
+    /**
+     * Serializer for {@link CatalogTableDescriptor}.
+     */
+    private static class TableDescriptorSerializer implements CatalogObjectSerializer<CatalogTableDescriptor> {
+        @Override
+        public CatalogTableDescriptor readFrom(int version, IgniteDataInput input) throws IOException {
+            CatalogDescriptorBase header = CatalogObjectDescriptor.SERIALIZER.readFrom(version, input);
+            CatalogTableSchemaVersions schemaVersions = CatalogTableSchemaVersions.SERIALIZER.readFrom(version, input);
+            List<CatalogTableColumnDescriptor> columns = readList(version, CatalogTableColumnDescriptor.SERIALIZER, input);
+
+            int schemaId = input.readInt();
+            int pkIndexId = input.readInt();
+            int zoneId = input.readInt();
+
+            List<String> primaryKeyColumns = readStringList(input);
+            List<String> colocationColumns = readStringList(input);
+
+            long creationToken = input.readLong();
+
+            return new CatalogTableDescriptor(
+                    header.id(),
+                    schemaId,
+                    pkIndexId,
+                    header.name(),
+                    zoneId,
+                    columns,
+                    primaryKeyColumns,
+                    colocationColumns,
+                    schemaVersions,
+                    header.updateToken(),
+                    creationToken
+            );
+        }
+
+        @Override
+        public void writeTo(CatalogTableDescriptor descriptor, int version, IgniteDataOutput output) throws IOException {
+            CatalogObjectDescriptor.SERIALIZER.writeTo(new CatalogDescriptorBase(descriptor), version, output);

Review Comment:
   ```suggestion
               output.writeInt(id);
               writeStringCollection(...);
               output.writeLong(updateToken());
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1467471178


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -219,4 +221,53 @@ public void updateToken(long updateToken) {
 
         this.creationToken = this.creationToken == INITIAL_CAUSALITY_TOKEN ? updateToken : this.creationToken;
     }
+
+    /**
+     * Serializer for {@link CatalogTableDescriptor}.
+     */
+    private static class TableDescriptorSerializer implements CatalogObjectSerializer<CatalogTableDescriptor> {
+        @Override
+        public CatalogTableDescriptor readFrom(int version, IgniteDataInput input) throws IOException {
+            CatalogDescriptorBase header = CatalogObjectDescriptor.SERIALIZER.readFrom(version, input);

Review Comment:
   Let's get rid of additional objects.
   ```suggestion
               int id = input.readInt()
               String name = input.readUtf();
               long input = readLong();
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1469677071


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptor.java:
##########
@@ -219,4 +221,53 @@ public void updateToken(long updateToken) {
 
         this.creationToken = this.creationToken == INITIAL_CAUSALITY_TOKEN ? updateToken : this.creationToken;
     }
+
+    /**
+     * Serializer for {@link CatalogTableDescriptor}.
+     */
+    private static class TableDescriptorSerializer implements CatalogObjectSerializer<CatalogTableDescriptor> {
+        @Override
+        public CatalogTableDescriptor readFrom(int version, IgniteDataInput input) throws IOException {
+            CatalogDescriptorBase header = CatalogObjectDescriptor.SERIALIZER.readFrom(version, input);
+            CatalogTableSchemaVersions schemaVersions = CatalogTableSchemaVersions.SERIALIZER.readFrom(version, input);
+            List<CatalogTableColumnDescriptor> columns = readList(version, CatalogTableColumnDescriptor.SERIALIZER, input);
+
+            int schemaId = input.readInt();
+            int pkIndexId = input.readInt();
+            int zoneId = input.readInt();
+
+            List<String> primaryKeyColumns = readStringList(input);
+            List<String> colocationColumns = readStringList(input);
+
+            long creationToken = input.readLong();
+
+            return new CatalogTableDescriptor(
+                    header.id(),
+                    schemaId,
+                    pkIndexId,
+                    header.name(),
+                    zoneId,
+                    columns,
+                    primaryKeyColumns,
+                    colocationColumns,
+                    schemaVersions,
+                    header.updateToken(),
+                    creationToken
+            );
+        }
+
+        @Override
+        public void writeTo(CatalogTableDescriptor descriptor, int version, IgniteDataOutput output) throws IOException {
+            CatalogObjectDescriptor.SERIALIZER.writeTo(new CatalogDescriptorBase(descriptor), version, output);
+            CatalogTableSchemaVersions.SERIALIZER.writeTo(descriptor.schemaVersions(), version, output);
+            writeList(descriptor.columns(), version, CatalogTableColumnDescriptor.SERIALIZER, output);
+
+            output.writeInt(descriptor.schemaId());
+            output.writeInt(descriptor.primaryKeyIndexId());
+            output.writeInt(descriptor.zoneId());
+            writeStringCollection(descriptor.primaryKeyColumns(), output);
+            writeStringCollection(descriptor.colocationColumns(), output);

Review Comment:
   Done, thanks.



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1469610978


##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/serialization/CatalogEntrySerializationCompatibilityTest.java:
##########
@@ -0,0 +1,429 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrows;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Objects;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests to verify catalog storage entries serialization compatibility.
+ */
+public class CatalogEntrySerializationCompatibilityTest {
+    private final CatalogEntrySerializerProvider serializerProvider = (typeId) -> {

Review Comment:
   I think 2 versions is enough. The test must checks next cases:
   * Marshaller of version X, must successfully marshal-unmarshal an entry.
   * Marshaller of version 2, must successfully unmarshal an entry marshalled by marshaller version 1.
   * Marshaller of version 1, must fails unmarshal an entry marshalled by marshaller version 2.
   



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1469676353


##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/serialization/CatalogEntrySerializationCompatibilityTest.java:
##########
@@ -0,0 +1,429 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrows;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Objects;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests to verify catalog storage entries serialization compatibility.
+ */
+public class CatalogEntrySerializationCompatibilityTest {
+    private final CatalogEntrySerializerProvider serializerProvider = (typeId) -> {

Review Comment:
   Done, test simplified, removed 3rd version of entry and serializer.
   
   ```
   Marshaller of version X, must successfully marshal-unmarshal an entry.
   Marshaller of version 2, must successfully unmarshal an entry marshalled by marshaller version 1.
   ```
   implemented in `readEntryInPreviousFormat`
   
   ```
   Marshaller of version X, must successfully marshal-unmarshal an entry.
   Marshaller of version 1, must fails unmarshal an entry marshalled by marshaller version 2.
   ```
   implemented in `forwardCompatibilityIsNotSupported`
   
   ```
   Serializer of version 2 can *write* entry in version 1 format
   ```
   implemented in `writeEntryInPreviousFormat`



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1469600730


##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/serialization/CatalogEntrySerializationCompatibilityTest.java:
##########
@@ -0,0 +1,429 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrows;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Objects;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests to verify catalog storage entries serialization compatibility.
+ */
+public class CatalogEntrySerializationCompatibilityTest {
+    private final CatalogEntrySerializerProvider serializerProvider = (typeId) -> {
+        switch (typeId) {
+            case 0:
+                return TestUpdateEntrySerializerV1.INSTANCE;
+
+            case 1:
+                return TestUpdateEntrySerializerV2.INSTANCE;
+
+            case 2:
+                return TestUpdateEntrySerializerV3.INSTANCE;
+
+            default:
+                throw new IllegalArgumentException("Unexpected type: " + typeId);
+        }
+    };
+
+    /**
+     * Checks whether the old version of an entity can be read by the new version's serializer.
+     */
+    @Test
+    public void readPreviousVersionEntry() {
+        // Writes a version 1 entry, reads it using the version 2 and 3 serializer.
+        {
+            TestUpdateEntry entryV1 = TestEntryFactory.create(1);
+
+            UpdateLogMarshaller marshallerV1 = new UpdateLogMarshallerImpl(1, serializerProvider);
+
+            List<UpdateEntry> entries = List.of(entryV1, entryV1);
+
+            VersionedUpdate update = new VersionedUpdate(1, 2, entries);
+
+            byte[] bytesV1 = marshallerV1.marshall(update);
+
+            VersionedUpdate deserialized = marshallerV1.unmarshall(bytesV1);
+            assertThat(deserialized.entries(), equalTo(entries));
+
+            UpdateLogMarshaller marshallerV2 = new UpdateLogMarshallerImpl(2, serializerProvider);
+            deserialized = marshallerV2.unmarshall(bytesV1);
+            assertThat(deserialized.entries(), equalTo(entries));
+
+            UpdateLogMarshaller marshallerV3 = new UpdateLogMarshallerImpl(3, serializerProvider);
+            deserialized = marshallerV3.unmarshall(bytesV1);
+            assertThat(deserialized.entries(), equalTo(entries));
+        }
+
+        // Writes a version 2 entry, reads it using the version 3 serializer.
+        {
+            TestUpdateEntryV2 entryV2 = TestEntryFactory.create(2);
+
+            UpdateLogMarshaller marshallerV2 = new UpdateLogMarshallerImpl(2, serializerProvider);
+
+            List<UpdateEntry> entries = List.of(entryV2, entryV2);
+            VersionedUpdate update = new VersionedUpdate(1, 2, entries);
+
+            byte[] bytesV2 = marshallerV2.marshall(update);
+
+            VersionedUpdate deserialized = marshallerV2.unmarshall(bytesV2);
+            assertThat(deserialized.entries(), equalTo(entries));
+
+            UpdateLogMarshaller marshallerV3 = new UpdateLogMarshallerImpl(3, serializerProvider);
+            deserialized = marshallerV3.unmarshall(bytesV2);
+            assertThat(deserialized.entries(), equalTo(entries));
+        }
+    }
+
+    /**
+     * Checks whether the entry can be written in the previous format.
+     */
+    @Test
+    public void writePreviousVersionEntry() {

Review Comment:
   What is the purpose of this test?
   If marshaller of version V1 doesn't knows about entry of V3, then it shouldn't be able to unmarshall it, isn't it?



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1468925037


##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/serialization/CatalogEntrySerializationCompatibilityTest.java:
##########
@@ -0,0 +1,371 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrows;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Objects;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests to verify catalog storage entries serialization compatibility.
+ */
+public class CatalogEntrySerializationCompatibilityTest {
+    private final CatalogEntrySerializerProvider serializerProvider = (ignore) -> TestUpdateEntrySerializer.INSTANCE;

Review Comment:
   From my point of view it doesn't affect the test case. But for clarity, I divided it into 3 different serializers.



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1479638321


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/storage/serialization/UpdateLogMarshallerImpl.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.storage.serialization;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+
+import org.apache.ignite.internal.catalog.storage.SnapshotEntry;
+import org.apache.ignite.internal.catalog.storage.UpdateLogEvent;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataInput;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.jetbrains.annotations.TestOnly;
+
+/**
+ * Marshaller of update log entries that uses custom serializer.
+ *
+ *<p>External serialization was implemented for two reasons:
+ *<ul>
+ * <li>Minimize the size of the serialized update log entry.</li>
+ * <li>Backward compatibility support when changing existing or adding new entry.</li>
+ * </ul>
+ * At the moment, the serialization format is specified manually.
+ *
+ *<p>While backward compatibility is not required, entities can change arbitrarily without changing the data format version.
+ * However, every change to an entry's fields requires a change to the serializer that serializes that entry.
+ *
+ *<p>Basic format description:
+ *<pre>
+ * (size) | description
+ * ------------------------------
+ *     2  | data format version ({@link #PROTOCOL_VERSION})
+ *     2  | entry type ({@link VersionedUpdate} or {@link SnapshotEntry})
+ *
+ * [fields of the corresponding event follow (for example, {@link VersionedUpdate}]
+ *

Review Comment:
   ```suggestion
    *     2  | entry type ({@link VersionedUpdate} or {@link SnapshotEntry})
    * <list> | fields of the corresponding event follow (for example, {@link VersionedUpdate}
    * </pre>
    * 
    * Versioned Update format description:
    * <pre>
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1479644576


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogColumnCollation.java:
##########
@@ -27,6 +27,8 @@ public enum CatalogColumnCollation {
     DESC_NULLS_FIRST(false, true),
     DESC_NULLS_LAST(false, false);
 
+    private static final int ASC_BIT_POS = 0;
+    private static final int NULLS_FIRST_BIT_POS = 1;

Review Comment:
   ```suggestion
       private static final int ASC_FLAG = 1;
       private static final int NULLS_FIRST_FLAG = 1 << 1;
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["AMashenkov (via GitHub)" <gi...@apache.org>]

AMashenkov commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1467490490


##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/serialization/CatalogSerializationUtils.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+import java.io.Serializable;
+import java.lang.reflect.Array;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.ignite.internal.util.ByteUtils;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Utility methods used for serializing catalog objects.
+ */
+public class CatalogSerializationUtils {
+    /** Reads nullable string. */
+    public static @Nullable String readNullableString(DataInput in) throws IOException {
+        if (!in.readBoolean()) {
+            return null;
+        }
+
+        return in.readUTF();
+    }
+
+    /** Writes nullable string. */
+    public static void writeNullableString(@Nullable String str, DataOutput out) throws IOException {
+        out.writeBoolean(str != null);
+
+        if (str != null) {
+            out.writeUTF(str);
+        }
+    }
+
+    /** Writes collection containing strings. */
+    public static void writeStringCollection(Collection<String> list, DataOutput out) throws IOException {
+        out.writeInt(list.size());
+
+        for (String item : list) {
+            out.writeUTF(item);
+        }
+    }
+
+    /** Reads list of strings. */
+    public static List<String> readStringList(DataInput in) throws IOException {

Review Comment:
   You can have a single method instead.
   ```suggestion
       public static <T extends Collection<String>> T readCollection(DataInput in, IntFunction<T> factory) throws IOException {
   ```



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

Re: [PR] IGNITE-21230 Custom serialization of catalog commands. [ignite-3]

["xtern (via GitHub)" <gi...@apache.org>]

xtern commented on code in PR #3058:
URL: https://github.com/apache/ignite-3/pull/3058#discussion_r1468925037


##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/serialization/CatalogEntrySerializationCompatibilityTest.java:
##########
@@ -0,0 +1,371 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.serialization;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrows;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Objects;
+import org.apache.ignite.internal.catalog.Catalog;
+import org.apache.ignite.internal.catalog.storage.UpdateEntry;
+import org.apache.ignite.internal.catalog.storage.VersionedUpdate;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.lang.MarshallerException;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests to verify catalog storage entries serialization compatibility.
+ */
+public class CatalogEntrySerializationCompatibilityTest {
+    private final CatalogEntrySerializerProvider serializerProvider = (ignore) -> TestUpdateEntrySerializer.INSTANCE;

Review Comment:
   From my point of view it doesn't affect the test case but breaks it. But for clarity, I divided it into 3 different serializers.



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