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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
