You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "rpuch (via GitHub)" <gi...@apache.org> on 2024/10/15 07:33:54 UTC
[PR] IGNITE-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
rpuch opened a new pull request, #4562:
URL: https://github.com/apache/ignite-3/pull/4562
https://issues.apache.org/jira/browse/IGNITE-23436
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802506810
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalNodeSerializer.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.cluster.management.topology.api;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.ignite.internal.network.ClusterNodeSerializer;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.internal.versioned.VersionedSerializer;
+import org.apache.ignite.network.ClusterNode;
+
+/**
+ * {@link VersionedSerializer} for {@link LogicalNode} instances.
+ */
+public class LogicalNodeSerializer extends VersionedSerializer<LogicalNode> {
+ /** Serializer instance. */
+ public static final LogicalNodeSerializer INSTANCE = new LogicalNodeSerializer();
+
+ private final ClusterNodeSerializer clusterNodeSerializer = ClusterNodeSerializer.INSTANCE;
+
+ @Override
+ protected void writeExternalData(LogicalNode node, IgniteDataOutput out) throws IOException {
+ clusterNodeSerializer.writeExternal(node, out);
+
+ writeStringToStringMap(node.userAttributes(), out);
+ writeStringToStringMap(node.systemAttributes(), out);
+
+ out.writeLength(node.storageProfiles().size());
+ for (String profile : node.storageProfiles()) {
+ out.writeUTF(profile);
+ }
+ }
+
+ private static void writeStringToStringMap(Map<String, String> map, IgniteDataOutput output) throws IOException {
+ output.writeLength(map.size());
+
+ for (Entry<String, String> entry : map.entrySet()) {
+ output.writeUTF(entry.getKey());
+ output.writeUTF(entry.getValue());
+ }
+ }
+
+ @Override
+ protected LogicalNode readExternalData(byte protoVer, IgniteDataInput in) throws IOException {
+ ClusterNode node = clusterNodeSerializer.readExternal(in);
+
+ Map<String, String> userAttributes = readStringToStringMap(in);
+ Map<String, String> systemAttributes = readStringToStringMap(in);
+ List<String> storageProfiles = readStringList(in);
+
+ return new LogicalNode(node, userAttributes, systemAttributes, storageProfiles);
+ }
+
+ private static Map<String, String> readStringToStringMap(IgniteDataInput in) throws IOException {
+ int size = in.readLength();
+
+ var map = new HashMap<String, String>(size);
+ for (int i = 0; i < size; i++) {
+ map.put(in.readUTF(), in.readUTF());
+ }
+
+ return Map.copyOf(map);
Review Comment:
It bites you rarely, but it might be difficult to debug it when it does bite you. Also, we don't create too many instances of `LogicalNode` (yet?), so I moved the defensive copying to the constructor. If we find it costs too much, we'll remove 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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch merged PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802497166
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/IgniteDataInput.java:
##########
@@ -95,6 +95,35 @@ public interface IgniteDataInput extends DataInput {
*/
int read(byte[] b, int off, int len) throws IOException;
+ /**
+ * Reads a length (that is, a non-negative integer that will most likely be small).
+ *
+ * @throws IOException If something goes wrong.
+ */
+ int readLength() throws IOException;
Review Comment:
Technically, it's not guaranteed to be small. It's most probable that it will be small. I'm not sure how to put this to words in a precise, but compact way...
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802490749
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalNode.java:
##########
@@ -71,28 +71,32 @@ public LogicalNode(UUID id, String name, NetworkAddress address) {
* @param userAttributes Node attributes defined in configuration.
*/
public LogicalNode(ClusterNode clusterNode, Map<String, String> userAttributes) {
- this(clusterNode, userAttributes, Collections.emptyMap(), Collections.emptyList());
+ this(clusterNode, userAttributes, emptyMap(), emptyList());
}
/**
* Constructor.
*
- * @param clusterNode Represents a node in a cluster.
+ * @param node Represents a node in a cluster.
* @param userAttributes Node attributes defined in configuration.
* @param systemAttributes Internal node attributes provided by system components at startup.
* @param storageProfiles List of storage profiles, which the node supports.
*/
public LogicalNode(
- ClusterNode clusterNode,
+ ClusterNode node,
Map<String, String> userAttributes,
Map<String, String> systemAttributes,
List<String> storageProfiles
) {
- super(clusterNode.id(), clusterNode.name(), clusterNode.address(), clusterNode.nodeMetadata());
-
- this.userAttributes = userAttributes == null ? Collections.emptyMap() : userAttributes;
- this.systemAttributes = systemAttributes == null ? Collections.emptyMap() : systemAttributes;
- this.storageProfiles = storageProfiles;
+ this(
+ node.id(),
+ node.name(),
+ node.address(),
+ node.nodeMetadata(),
+ userAttributes == null ? emptyMap() : userAttributes,
Review Comment:
I tried to make sure we never pass nulls to parameters of this constructor (which was the case) and removed the null checks.
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802507834
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalNodeSerializer.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.cluster.management.topology.api;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.ignite.internal.network.ClusterNodeSerializer;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.internal.versioned.VersionedSerializer;
+import org.apache.ignite.network.ClusterNode;
+
+/**
+ * {@link VersionedSerializer} for {@link LogicalNode} instances.
+ */
+public class LogicalNodeSerializer extends VersionedSerializer<LogicalNode> {
+ /** Serializer instance. */
+ public static final LogicalNodeSerializer INSTANCE = new LogicalNodeSerializer();
+
+ private final ClusterNodeSerializer clusterNodeSerializer = ClusterNodeSerializer.INSTANCE;
+
+ @Override
+ protected void writeExternalData(LogicalNode node, IgniteDataOutput out) throws IOException {
+ clusterNodeSerializer.writeExternal(node, out);
+
+ writeStringToStringMap(node.userAttributes(), out);
+ writeStringToStringMap(node.systemAttributes(), out);
+
+ out.writeLength(node.storageProfiles().size());
+ for (String profile : node.storageProfiles()) {
+ out.writeUTF(profile);
+ }
+ }
+
+ private static void writeStringToStringMap(Map<String, String> map, IgniteDataOutput output) throws IOException {
+ output.writeLength(map.size());
+
+ for (Entry<String, String> entry : map.entrySet()) {
+ output.writeUTF(entry.getKey());
+ output.writeUTF(entry.getValue());
+ }
+ }
+
+ @Override
+ protected LogicalNode readExternalData(byte protoVer, IgniteDataInput in) throws IOException {
+ ClusterNode node = clusterNodeSerializer.readExternal(in);
+
+ Map<String, String> userAttributes = readStringToStringMap(in);
+ Map<String, String> systemAttributes = readStringToStringMap(in);
+ List<String> storageProfiles = readStringList(in);
+
+ return new LogicalNode(node, userAttributes, systemAttributes, storageProfiles);
+ }
+
+ private static Map<String, String> readStringToStringMap(IgniteDataInput in) throws IOException {
+ int size = in.readLength();
+
+ var map = new HashMap<String, String>(size);
+ for (int i = 0; i < size; i++) {
+ map.put(in.readUTF(), in.readUTF());
+ }
+
+ return Map.copyOf(map);
+ }
+
+ private static List<String> readStringList(IgniteDataInput in) throws IOException {
+ int size = in.readLength();
+
+ var list = new ArrayList<String>(size);
+ for (int i = 0; i < size; i++) {
+ list.add(in.readUTF());
+ }
+
+ return List.copyOf(list);
Review Comment:
Moved creation of defensive copies to the constructor (see the comments above)
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802489269
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/NaiveVarInts.java:
##########
@@ -20,12 +20,17 @@
import java.io.DataInput;
import java.io.DataOutput;
import java.io.IOException;
+import org.apache.ignite.internal.util.VarIntUtils;
/**
- * Utils to read/write variable length ints.
+ * Utils to read/write naive variable length ints.
+ *
+ * <p>The 'naivety' relates to a simple algorithm. The naive varints produce more compact results than the 'general purpose' varints
+ * (see {@link VarIntUtils}) when the input integer is approximately between 127 and 254, so they seem to be more appropriate
+ * for small values (like lengths of short strings and collections).
*/
-public class VarInts {
- private VarInts() {
+public class NaiveVarInts {
Review Comment:
You are probably talking about VLQ https://en.wikipedia.org/wiki/Variable-length_quantity
But you could, technically, encode integers in a variable number of bytes, in another way (that is implemented in this class). If the name (naive var ints) is confusing, let's invent another name. Do you have suggestions?
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802798403
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/NaiveVarInts.java:
##########
@@ -20,12 +20,17 @@
import java.io.DataInput;
import java.io.DataOutput;
import java.io.IOException;
+import org.apache.ignite.internal.util.VarIntUtils;
/**
- * Utils to read/write variable length ints.
+ * Utils to read/write naive variable length ints.
+ *
+ * <p>The 'naivety' relates to a simple algorithm. The naive varints produce more compact results than the 'general purpose' varints
+ * (see {@link VarIntUtils}) when the input integer is approximately between 127 and 254, so they seem to be more appropriate
+ * for small values (like lengths of short strings and collections).
*/
-public class VarInts {
- private VarInts() {
+public class NaiveVarInts {
Review Comment:
After a bit of thinking, I realized that the advantage that `NaiveVarInts` provide is so tiny that it does not justify the complexity of having 2 implementations. I switched to usual varints and added a TODO to remove `NaiveVarInts` altogether (it will require more actions than just removing the class, so it's better to do it in a separate PR).
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802469363
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/IgniteDataInput.java:
##########
@@ -95,6 +95,35 @@ public interface IgniteDataInput extends DataInput {
*/
int read(byte[] b, int off, int len) throws IOException;
+ /**
+ * Reads a length (that is, a non-negative integer that will most likely be small).
+ *
+ * @throws IOException If something goes wrong.
+ */
+ int readLength() throws IOException;
+
+ /**
+ * Reads a long value as a varint.
+ *
+ * @throws IOException If something goes wrong.
+ * @see IgniteDataOutput#writeVarInt(long)
+ */
+ long readVarInt() throws IOException;
+
+ /**
+ * Reads an int value as a varint.
+ *
+ * @throws IOException If something goes wrong.
+ * @see IgniteDataOutput#writeVarInt(long)
+ */
+ default int readVarIntAsInt() throws IOException {
+ long val = readVarInt();
+ if (val < Integer.MIN_VALUE || val > Integer.MAX_VALUE) {
+ throw new IOException("The value is expected to fit into int range, but it doesn't: " + val);
+ }
+ return (int) val;
Review Comment:
`Math#toExactInt()` does not include information about the exact value in the error message, so explicit handling seems better
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802483439
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalNodeSerializer.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.cluster.management.topology.api;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.ignite.internal.network.ClusterNodeSerializer;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.internal.versioned.VersionedSerializer;
+import org.apache.ignite.network.ClusterNode;
+
+/**
+ * {@link VersionedSerializer} for {@link LogicalNode} instances.
+ */
+public class LogicalNodeSerializer extends VersionedSerializer<LogicalNode> {
+ /** Serializer instance. */
+ public static final LogicalNodeSerializer INSTANCE = new LogicalNodeSerializer();
+
+ private final ClusterNodeSerializer clusterNodeSerializer = ClusterNodeSerializer.INSTANCE;
+
+ @Override
+ protected void writeExternalData(LogicalNode node, IgniteDataOutput out) throws IOException {
+ clusterNodeSerializer.writeExternal(node, out);
+
+ writeStringToStringMap(node.userAttributes(), out);
+ writeStringToStringMap(node.systemAttributes(), out);
+
+ out.writeLength(node.storageProfiles().size());
+ for (String profile : node.storageProfiles()) {
+ out.writeUTF(profile);
+ }
+ }
+
+ private static void writeStringToStringMap(Map<String, String> map, IgniteDataOutput output) throws IOException {
+ output.writeLength(map.size());
+
+ for (Entry<String, String> entry : map.entrySet()) {
+ output.writeUTF(entry.getKey());
+ output.writeUTF(entry.getValue());
+ }
+ }
+
+ @Override
+ protected LogicalNode readExternalData(byte protoVer, IgniteDataInput in) throws IOException {
+ ClusterNode node = clusterNodeSerializer.readExternal(in);
+
+ Map<String, String> userAttributes = readStringToStringMap(in);
+ Map<String, String> systemAttributes = readStringToStringMap(in);
+ List<String> storageProfiles = readStringList(in);
+
+ return new LogicalNode(node, userAttributes, systemAttributes, storageProfiles);
+ }
+
+ private static Map<String, String> readStringToStringMap(IgniteDataInput in) throws IOException {
+ int size = in.readLength();
+
+ var map = new HashMap<String, String>(size);
+ for (int i = 0; i < size; i++) {
+ map.put(in.readUTF(), in.readUTF());
+ }
+
+ return Map.copyOf(map);
Review Comment:
1. I think the problem of passing mutable maps to a constructor and later changing them is very rare and I've never met it in our codebase
2. If you insist on protecting ourselves, then creating a copy inside the constructor is the way to go.
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802501022
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/IgniteDataInput.java:
##########
@@ -95,6 +95,35 @@ public interface IgniteDataInput extends DataInput {
*/
int read(byte[] b, int off, int len) throws IOException;
+ /**
+ * Reads a length (that is, a non-negative integer that will most likely be small).
+ *
+ * @throws IOException If something goes wrong.
+ */
+ int readLength() throws IOException;
Review Comment:
I tried to improve javadocs by mentioning that it's about string/collection lengths
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802481182
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/IgniteDataInput.java:
##########
@@ -95,6 +95,35 @@ public interface IgniteDataInput extends DataInput {
*/
int read(byte[] b, int off, int len) throws IOException;
+ /**
+ * Reads a length (that is, a non-negative integer that will most likely be small).
+ *
+ * @throws IOException If something goes wrong.
+ */
+ int readLength() throws IOException;
Review Comment:
maybe we should call it `readSmallInt`? Current approach is also ok, though
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802471629
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/IgniteDataInput.java:
##########
@@ -95,6 +95,35 @@ public interface IgniteDataInput extends DataInput {
*/
int read(byte[] b, int off, int len) throws IOException;
+ /**
+ * Reads a length (that is, a non-negative integer that will most likely be small).
+ *
+ * @throws IOException If something goes wrong.
+ */
+ int readLength() throws IOException;
Review Comment:
Because it is about a specific subset of unsigned ints: lengths. Lengths have a specific distribution of values (the smaller the value, the more the probability that it will be used) as shorter strings and collections are more probable than long ones. This allows to use a specific encoding method (namely, 'naive var int') to exploit this distribution.
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802504587
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/NaiveVarInts.java:
##########
@@ -20,12 +20,17 @@
import java.io.DataInput;
import java.io.DataOutput;
import java.io.IOException;
+import org.apache.ignite.internal.util.VarIntUtils;
/**
- * Utils to read/write variable length ints.
+ * Utils to read/write naive variable length ints.
+ *
+ * <p>The 'naivety' relates to a simple algorithm. The naive varints produce more compact results than the 'general purpose' varints
+ * (see {@link VarIntUtils}) when the input integer is approximately between 127 and 254, so they seem to be more appropriate
+ * for small values (like lengths of short strings and collections).
*/
-public class VarInts {
- private VarInts() {
+public class NaiveVarInts {
Review Comment:
Isn't what we have in `VarIntUtils` a variant of VLQ?
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802529715
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/NaiveVarInts.java:
##########
@@ -20,12 +20,17 @@
import java.io.DataInput;
import java.io.DataOutput;
import java.io.IOException;
+import org.apache.ignite.internal.util.VarIntUtils;
/**
- * Utils to read/write variable length ints.
+ * Utils to read/write naive variable length ints.
+ *
+ * <p>The 'naivety' relates to a simple algorithm. The naive varints produce more compact results than the 'general purpose' varints
+ * (see {@link VarIntUtils}) when the input integer is approximately between 127 and 254, so they seem to be more appropriate
+ * for small values (like lengths of short strings and collections).
*/
-public class VarInts {
- private VarInts() {
+public class NaiveVarInts {
Review Comment:
yes, but I think that when you say "varint" to anyone else related to the project, they will think of *this particular* encoding variant. So having "naive varint" is very confusing (at least to me)
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802499093
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/IgniteDataInput.java:
##########
@@ -95,6 +95,35 @@ public interface IgniteDataInput extends DataInput {
*/
int read(byte[] b, int off, int len) throws IOException;
+ /**
+ * Reads a length (that is, a non-negative integer that will most likely be small).
+ *
+ * @throws IOException If something goes wrong.
+ */
+ int readLength() throws IOException;
Review Comment:
ok, let's leave this variant
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1800614935
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalNode.java:
##########
@@ -71,28 +71,32 @@ public LogicalNode(UUID id, String name, NetworkAddress address) {
* @param userAttributes Node attributes defined in configuration.
*/
public LogicalNode(ClusterNode clusterNode, Map<String, String> userAttributes) {
- this(clusterNode, userAttributes, Collections.emptyMap(), Collections.emptyList());
+ this(clusterNode, userAttributes, emptyMap(), emptyList());
}
/**
* Constructor.
*
- * @param clusterNode Represents a node in a cluster.
+ * @param node Represents a node in a cluster.
* @param userAttributes Node attributes defined in configuration.
* @param systemAttributes Internal node attributes provided by system components at startup.
* @param storageProfiles List of storage profiles, which the node supports.
*/
public LogicalNode(
- ClusterNode clusterNode,
+ ClusterNode node,
Map<String, String> userAttributes,
Map<String, String> systemAttributes,
List<String> storageProfiles
) {
- super(clusterNode.id(), clusterNode.name(), clusterNode.address(), clusterNode.nodeMetadata());
-
- this.userAttributes = userAttributes == null ? Collections.emptyMap() : userAttributes;
- this.systemAttributes = systemAttributes == null ? Collections.emptyMap() : systemAttributes;
- this.storageProfiles = storageProfiles;
+ this(
+ node.id(),
+ node.name(),
+ node.address(),
+ node.nodeMetadata(),
+ userAttributes == null ? emptyMap() : userAttributes,
Review Comment:
Can it be `null`? It's not marked as `nullable`
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalNodeSerializer.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.cluster.management.topology.api;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.ignite.internal.network.ClusterNodeSerializer;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.internal.versioned.VersionedSerializer;
+import org.apache.ignite.network.ClusterNode;
+
+/**
+ * {@link VersionedSerializer} for {@link LogicalNode} instances.
+ */
+public class LogicalNodeSerializer extends VersionedSerializer<LogicalNode> {
+ /** Serializer instance. */
+ public static final LogicalNodeSerializer INSTANCE = new LogicalNodeSerializer();
+
+ private final ClusterNodeSerializer clusterNodeSerializer = ClusterNodeSerializer.INSTANCE;
+
+ @Override
+ protected void writeExternalData(LogicalNode node, IgniteDataOutput out) throws IOException {
+ clusterNodeSerializer.writeExternal(node, out);
+
+ writeStringToStringMap(node.userAttributes(), out);
+ writeStringToStringMap(node.systemAttributes(), out);
+
+ out.writeLength(node.storageProfiles().size());
+ for (String profile : node.storageProfiles()) {
+ out.writeUTF(profile);
+ }
+ }
+
+ private static void writeStringToStringMap(Map<String, String> map, IgniteDataOutput output) throws IOException {
+ output.writeLength(map.size());
+
+ for (Entry<String, String> entry : map.entrySet()) {
+ output.writeUTF(entry.getKey());
+ output.writeUTF(entry.getValue());
+ }
+ }
+
+ @Override
+ protected LogicalNode readExternalData(byte protoVer, IgniteDataInput in) throws IOException {
+ ClusterNode node = clusterNodeSerializer.readExternal(in);
+
+ Map<String, String> userAttributes = readStringToStringMap(in);
+ Map<String, String> systemAttributes = readStringToStringMap(in);
+ List<String> storageProfiles = readStringList(in);
+
+ return new LogicalNode(node, userAttributes, systemAttributes, storageProfiles);
+ }
+
+ private static Map<String, String> readStringToStringMap(IgniteDataInput in) throws IOException {
+ int size = in.readLength();
+
+ var map = new HashMap<String, String>(size);
+ for (int i = 0; i < size; i++) {
+ map.put(in.readUTF(), in.readUTF());
+ }
+
+ return Map.copyOf(map);
Review Comment:
Why are you creating copies here?
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/NaiveVarInts.java:
##########
@@ -20,12 +20,17 @@
import java.io.DataInput;
import java.io.DataOutput;
import java.io.IOException;
+import org.apache.ignite.internal.util.VarIntUtils;
/**
- * Utils to read/write variable length ints.
+ * Utils to read/write naive variable length ints.
+ *
+ * <p>The 'naivety' relates to a simple algorithm. The naive varints produce more compact results than the 'general purpose' varints
+ * (see {@link VarIntUtils}) when the input integer is approximately between 127 and 254, so they seem to be more appropriate
+ * for small values (like lengths of short strings and collections).
*/
-public class VarInts {
- private VarInts() {
+public class NaiveVarInts {
Review Comment:
This is very confusing, why does this class even has `varint` in its name? I think everyone understands a particular encoding under this name
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/IgniteDataInput.java:
##########
@@ -95,6 +95,35 @@ public interface IgniteDataInput extends DataInput {
*/
int read(byte[] b, int off, int len) throws IOException;
+ /**
+ * Reads a length (that is, a non-negative integer that will most likely be small).
+ *
+ * @throws IOException If something goes wrong.
+ */
+ int readLength() throws IOException;
+
+ /**
+ * Reads a long value as a varint.
+ *
+ * @throws IOException If something goes wrong.
+ * @see IgniteDataOutput#writeVarInt(long)
+ */
+ long readVarInt() throws IOException;
+
+ /**
+ * Reads an int value as a varint.
+ *
+ * @throws IOException If something goes wrong.
+ * @see IgniteDataOutput#writeVarInt(long)
+ */
+ default int readVarIntAsInt() throws IOException {
+ long val = readVarInt();
+ if (val < Integer.MIN_VALUE || val > Integer.MAX_VALUE) {
+ throw new IOException("The value is expected to fit into int range, but it doesn't: " + val);
+ }
+ return (int) val;
Review Comment:
Can we use `Math.toIntExact` instead?
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalTopologySnapshotSerializer.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.cluster.management.topology.api;
+
+import java.io.IOException;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.UUID;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.internal.versioned.VersionedSerializer;
+
+/**
+ * {@link VersionedSerializer} for {@link LogicalTopologySnapshot} instances.
+ */
+public class LogicalTopologySnapshotSerializer extends VersionedSerializer<LogicalTopologySnapshot> {
+ /** Serializer instance. */
+ public static final LogicalTopologySnapshotSerializer INSTANCE = new LogicalTopologySnapshotSerializer();
+
+ private final LogicalNodeSerializer logicalNodeSerializer = new LogicalNodeSerializer();
+
+ @Override
+ protected void writeExternalData(LogicalTopologySnapshot snapshot, IgniteDataOutput out) throws IOException {
+ out.writeVarInt(snapshot.version());
+
+ out.writeLength(snapshot.nodes().size());
+ for (LogicalNode node : snapshot.nodes()) {
+ logicalNodeSerializer.writeExternal(node, out);
+ }
+
+ out.writeUuid(snapshot.clusterId());
+ }
+
+ @Override
+ protected LogicalTopologySnapshot readExternalData(byte protoVer, IgniteDataInput in) throws IOException {
+ long version = in.readVarInt();
+
+ int nodesCount = in.readLength();
+ Set<LogicalNode> nodes = new HashSet<>(nodesCount);
Review Comment:
you need to use `IgniteUtils#capacity` instead of directly passing the capacity to a HashSet or a HashMap
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalNodeSerializer.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.cluster.management.topology.api;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.ignite.internal.network.ClusterNodeSerializer;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.internal.versioned.VersionedSerializer;
+import org.apache.ignite.network.ClusterNode;
+
+/**
+ * {@link VersionedSerializer} for {@link LogicalNode} instances.
+ */
+public class LogicalNodeSerializer extends VersionedSerializer<LogicalNode> {
+ /** Serializer instance. */
+ public static final LogicalNodeSerializer INSTANCE = new LogicalNodeSerializer();
+
+ private final ClusterNodeSerializer clusterNodeSerializer = ClusterNodeSerializer.INSTANCE;
+
+ @Override
+ protected void writeExternalData(LogicalNode node, IgniteDataOutput out) throws IOException {
+ clusterNodeSerializer.writeExternal(node, out);
+
+ writeStringToStringMap(node.userAttributes(), out);
+ writeStringToStringMap(node.systemAttributes(), out);
+
+ out.writeLength(node.storageProfiles().size());
+ for (String profile : node.storageProfiles()) {
+ out.writeUTF(profile);
+ }
+ }
+
+ private static void writeStringToStringMap(Map<String, String> map, IgniteDataOutput output) throws IOException {
+ output.writeLength(map.size());
+
+ for (Entry<String, String> entry : map.entrySet()) {
+ output.writeUTF(entry.getKey());
+ output.writeUTF(entry.getValue());
+ }
+ }
+
+ @Override
+ protected LogicalNode readExternalData(byte protoVer, IgniteDataInput in) throws IOException {
+ ClusterNode node = clusterNodeSerializer.readExternal(in);
+
+ Map<String, String> userAttributes = readStringToStringMap(in);
+ Map<String, String> systemAttributes = readStringToStringMap(in);
+ List<String> storageProfiles = readStringList(in);
+
+ return new LogicalNode(node, userAttributes, systemAttributes, storageProfiles);
+ }
+
+ private static Map<String, String> readStringToStringMap(IgniteDataInput in) throws IOException {
+ int size = in.readLength();
+
+ var map = new HashMap<String, String>(size);
+ for (int i = 0; i < size; i++) {
+ map.put(in.readUTF(), in.readUTF());
+ }
+
+ return Map.copyOf(map);
+ }
+
+ private static List<String> readStringList(IgniteDataInput in) throws IOException {
+ int size = in.readLength();
+
+ var list = new ArrayList<String>(size);
+ for (int i = 0; i < size; i++) {
+ list.add(in.readUTF());
+ }
+
+ return List.copyOf(list);
Review Comment:
Same question
##########
modules/core/src/main/java/org/apache/ignite/internal/versioned/VersionedSerializer.java:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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.versioned;
+
+import java.io.IOException;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+
+/**
+ * Serializes and deserializes objects in a versioned way: that is, includes version to make it possible to deserialize objects serialized
+ * (and persisted) in earlier versions later when the corresponding class'es structure has changed.
+ *
+ * <p>It is supposed to be used for the cases when some object needs to be persisted to be stored for some time (maybe, a long time).
+ *
+ * <p>If a format is changed (for example, a new field is added), the version returned by {@link #getProtocolVersion()} has
+ * to be incremented.
+ */
+public abstract class VersionedSerializer<T> {
+ /** Magic number to detect correct serialized objects. */
+ private static final int MAGIC = 0x43BEEF00;
+
+ /**
+ * Returns protocol version.
+ */
+ protected byte getProtocolVersion() {
+ return 1;
+ }
+
+ /**
+ * Save object's specific (that is, ignoring the signature and version) data content.
+ *
+ * @param object object to write.
+ * @param out Output to write data content.
+ * @throws IOException If an I/O error occurs.
+ */
+ protected abstract void writeExternalData(T object, IgniteDataOutput out) throws IOException;
+
+ /**
+ * Writes an object to an output, including a signature and version.
+ *
+ * @param object Object to write.
+ * @param out Output to which to write.
+ * @throws IOException If an I/O error occurs.
+ */
+ public final void writeExternal(T object, IgniteDataOutput out) throws IOException {
+ int hdr = MAGIC + (getProtocolVersion() & 0xFF);
Review Comment:
Not very important, but we can use `Byte.toUnsignedInt()` here
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalTopologySnapshotSerializer.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.cluster.management.topology.api;
+
+import java.io.IOException;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.UUID;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.internal.versioned.VersionedSerializer;
+
+/**
+ * {@link VersionedSerializer} for {@link LogicalTopologySnapshot} instances.
+ */
+public class LogicalTopologySnapshotSerializer extends VersionedSerializer<LogicalTopologySnapshot> {
+ /** Serializer instance. */
+ public static final LogicalTopologySnapshotSerializer INSTANCE = new LogicalTopologySnapshotSerializer();
+
+ private final LogicalNodeSerializer logicalNodeSerializer = new LogicalNodeSerializer();
Review Comment:
Why aren't you using the singleton here?
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/IgniteDataInput.java:
##########
@@ -95,6 +95,35 @@ public interface IgniteDataInput extends DataInput {
*/
int read(byte[] b, int off, int len) throws IOException;
+ /**
+ * Reads a length (that is, a non-negative integer that will most likely be small).
+ *
+ * @throws IOException If something goes wrong.
+ */
+ int readLength() throws IOException;
Review Comment:
Why is it called "length", and not "readUnsignedInt"?
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802477130
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalNodeSerializer.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.cluster.management.topology.api;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.ignite.internal.network.ClusterNodeSerializer;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.internal.versioned.VersionedSerializer;
+import org.apache.ignite.network.ClusterNode;
+
+/**
+ * {@link VersionedSerializer} for {@link LogicalNode} instances.
+ */
+public class LogicalNodeSerializer extends VersionedSerializer<LogicalNode> {
+ /** Serializer instance. */
+ public static final LogicalNodeSerializer INSTANCE = new LogicalNodeSerializer();
+
+ private final ClusterNodeSerializer clusterNodeSerializer = ClusterNodeSerializer.INSTANCE;
+
+ @Override
+ protected void writeExternalData(LogicalNode node, IgniteDataOutput out) throws IOException {
+ clusterNodeSerializer.writeExternal(node, out);
+
+ writeStringToStringMap(node.userAttributes(), out);
+ writeStringToStringMap(node.systemAttributes(), out);
+
+ out.writeLength(node.storageProfiles().size());
+ for (String profile : node.storageProfiles()) {
+ out.writeUTF(profile);
+ }
+ }
+
+ private static void writeStringToStringMap(Map<String, String> map, IgniteDataOutput output) throws IOException {
+ output.writeLength(map.size());
+
+ for (Entry<String, String> entry : map.entrySet()) {
+ output.writeUTF(entry.getKey());
+ output.writeUTF(entry.getValue());
+ }
+ }
+
+ @Override
+ protected LogicalNode readExternalData(byte protoVer, IgniteDataInput in) throws IOException {
+ ClusterNode node = clusterNodeSerializer.readExternal(in);
+
+ Map<String, String> userAttributes = readStringToStringMap(in);
+ Map<String, String> systemAttributes = readStringToStringMap(in);
+ List<String> storageProfiles = readStringList(in);
+
+ return new LogicalNode(node, userAttributes, systemAttributes, storageProfiles);
+ }
+
+ private static Map<String, String> readStringToStringMap(IgniteDataInput in) throws IOException {
+ int size = in.readLength();
+
+ var map = new HashMap<String, String>(size);
+ for (int i = 0; i < size; i++) {
+ map.put(in.readUTF(), in.readUTF());
+ }
+
+ return Map.copyOf(map);
Review Comment:
`LogicalNode` just returns the maps and list it's given to its constructor as is, so if they are modifiable, we can get in trouble.
1. We can make copies in `LogicalNode` constructor
2. We can make copies in its serializer
3. Or we can add an unmodifiable wrapper when storing them
Of these, I like option 1 more than others (for 2, we can forget to make a copy outside of the constructor, and for 3, it will create an unnecessary object for the cases when an immutable collection is passed to the constructor).
What do you think?
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802493116
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/NaiveVarInts.java:
##########
@@ -20,12 +20,17 @@
import java.io.DataInput;
import java.io.DataOutput;
import java.io.IOException;
+import org.apache.ignite.internal.util.VarIntUtils;
/**
- * Utils to read/write variable length ints.
+ * Utils to read/write naive variable length ints.
+ *
+ * <p>The 'naivety' relates to a simple algorithm. The naive varints produce more compact results than the 'general purpose' varints
+ * (see {@link VarIntUtils}) when the input integer is approximately between 127 and 254, so they seem to be more appropriate
+ * for small values (like lengths of short strings and collections).
*/
-public class VarInts {
- private VarInts() {
+public class NaiveVarInts {
Review Comment:
> You are probably talking about VLQ
No, I was talking about the encoding implemented in `VarIntUtils`. As for the name for this class, I would suggest discussing it with the team, because I don't have any constructive suggestions =(
--
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-23436 Do not use ByteUtils#toBytes to persist objects related to CMG [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4562:
URL: https://github.com/apache/ignite-3/pull/4562#discussion_r1802506810
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalNodeSerializer.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.cluster.management.topology.api;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.apache.ignite.internal.network.ClusterNodeSerializer;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.internal.versioned.VersionedSerializer;
+import org.apache.ignite.network.ClusterNode;
+
+/**
+ * {@link VersionedSerializer} for {@link LogicalNode} instances.
+ */
+public class LogicalNodeSerializer extends VersionedSerializer<LogicalNode> {
+ /** Serializer instance. */
+ public static final LogicalNodeSerializer INSTANCE = new LogicalNodeSerializer();
+
+ private final ClusterNodeSerializer clusterNodeSerializer = ClusterNodeSerializer.INSTANCE;
+
+ @Override
+ protected void writeExternalData(LogicalNode node, IgniteDataOutput out) throws IOException {
+ clusterNodeSerializer.writeExternal(node, out);
+
+ writeStringToStringMap(node.userAttributes(), out);
+ writeStringToStringMap(node.systemAttributes(), out);
+
+ out.writeLength(node.storageProfiles().size());
+ for (String profile : node.storageProfiles()) {
+ out.writeUTF(profile);
+ }
+ }
+
+ private static void writeStringToStringMap(Map<String, String> map, IgniteDataOutput output) throws IOException {
+ output.writeLength(map.size());
+
+ for (Entry<String, String> entry : map.entrySet()) {
+ output.writeUTF(entry.getKey());
+ output.writeUTF(entry.getValue());
+ }
+ }
+
+ @Override
+ protected LogicalNode readExternalData(byte protoVer, IgniteDataInput in) throws IOException {
+ ClusterNode node = clusterNodeSerializer.readExternal(in);
+
+ Map<String, String> userAttributes = readStringToStringMap(in);
+ Map<String, String> systemAttributes = readStringToStringMap(in);
+ List<String> storageProfiles = readStringList(in);
+
+ return new LogicalNode(node, userAttributes, systemAttributes, storageProfiles);
+ }
+
+ private static Map<String, String> readStringToStringMap(IgniteDataInput in) throws IOException {
+ int size = in.readLength();
+
+ var map = new HashMap<String, String>(size);
+ for (int i = 0; i < size; i++) {
+ map.put(in.readUTF(), in.readUTF());
+ }
+
+ return Map.copyOf(map);
Review Comment:
It bites you rarely, but it might be difficult to debug it when it does bite you. Also, we don't creeate too many instances of `LogicalNode` (yet?), so I moved the defensive copying to the constructor. If we find it costs too much, we'll remove 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