SammyVimes commented on a change in pull request #670:
URL: https://github.com/apache/ignite-3/pull/670#discussion_r809851247
##########
File path:
modules/network/src/main/java/org/apache/ignite/internal/network/message/ClassDescriptorMessage.java
##########
@@ -29,6 +28,17 @@
/** Message for the {@link ClassDescriptor}. */
@Transferable(NetworkMessageTypes.CLASS_DESCRIPTOR_MESSAGE)
public interface ClassDescriptorMessage extends NetworkMessage {
+ int IS_PRIMITIVE_MASK = 1;
Review comment:
Maybe it would be more clear to write it like `1 << 1` and so on?
##########
File path:
modules/network/src/main/java/org/apache/ignite/internal/network/serialization/ClassDescriptor.java
##########
@@ -52,21 +52,28 @@
@Nullable
private final ClassDescriptor superClassDescriptor;
+ /**
+ * Component type descriptor (only present for arrays).
+ */
+ @Nullable
+ private final ClassDescriptor componentTypeDescriptor;
+
+ private final boolean isPrimitive;
+ private final boolean isArray;
+ private final boolean isRuntimeEnum;
+ private final boolean isRuntimeTypeKnownUpfront;
+
/**
* List of the declared class fields' descriptors.
*/
- @NotNull
private final List<FieldDescriptor> fields;
/**
* How the class is to be serialized.
*/
private final Serialization serialization;
- /**
- * Whether the class is final.
- */
- private final boolean isFinal;
+ private final List<MergedField> mergedFields;
Review comment:
And this field too
##########
File path: parent/pom.xml
##########
@@ -87,6 +87,7 @@
<caffeine.version>3.0.4</caffeine.version>
<fastutil.version>8.5.6</fastutil.version>
<kryo.version>4.0.1</kryo.version>
+ <byte.buddy.version>1.12.8</byte.buddy.version>
Review comment:
```suggestion
<bytebuddy.version>1.12.8</bytebuddy.version>
```
##########
File path:
modules/network/src/main/java/org/apache/ignite/internal/network/serialization/PerSessionSerializationService.java
##########
@@ -182,6 +184,34 @@ public PerSessionSerializationService(@NotNull
SerializationService serializatio
return messages;
}
+ private byte fieldFlags(FieldDescriptor fieldDescriptor) {
+ int bits = condMask(fieldDescriptor.isUnshared(),
FieldDescriptorMessage.UNSHARED_MASK)
+ | condMask(fieldDescriptor.isPrimitive(),
FieldDescriptorMessage.IS_PRIMITIVE)
+ | condMask(fieldDescriptor.isRuntimeTypeKnownUpfront(),
FieldDescriptorMessage.IS_RUNTIME_TYPE_KNOWN_UPFRONT);
+ return (byte) bits;
+ }
+
+ private byte serializationAttributeFlags(Serialization serialization) {
+ int bits = condMask(serialization.hasWriteObject(),
ClassDescriptorMessage.HAS_WRITE_OBJECT_MASK)
+ | condMask(serialization.hasReadObject(),
ClassDescriptorMessage.HAS_READ_OBJECT_MASK)
+ | condMask(serialization.hasReadObjectNoData(),
ClassDescriptorMessage.HAS_READ_OBJECT_NO_DATA_MASK)
+ | condMask(serialization.hasWriteReplace(),
ClassDescriptorMessage.HAS_WRITE_REPLACE_MASK)
+ | condMask(serialization.hasReadResolve(),
ClassDescriptorMessage.HAS_READ_RESOLVE_MASK);
+ return (byte) bits;
+ }
+
+ private int condMask(boolean value, int mask) {
Review comment:
this method is always called multiple times for all the flags, maybe it
would look better if it was something like:
```
condMask(boolean[] values, int[] masks)
```
It's arguably better, though
##########
File path:
modules/network/src/main/java/org/apache/ignite/internal/network/serialization/ClassDescriptor.java
##########
@@ -52,21 +52,28 @@
@Nullable
private final ClassDescriptor superClassDescriptor;
+ /**
+ * Component type descriptor (only present for arrays).
+ */
+ @Nullable
+ private final ClassDescriptor componentTypeDescriptor;
+
+ private final boolean isPrimitive;
Review comment:
Looks odd that this fields are declared without newlines and javadocs
(as opposed to other fields)
##########
File path: parent/pom.xml
##########
@@ -649,6 +650,12 @@
<artifactId>fastutil-core</artifactId>
<version>${fastutil.version}</version>
</dependency>
+
+ <dependency>
+ <groupId>net.bytebuddy</groupId>
+ <artifactId>byte-buddy</artifactId>
+ <version>${byte.buddy.version}</version>
Review comment:
```suggestion
<version>${bytebuddy.version}</version>
```
##########
File path:
modules/network/src/main/java/org/apache/ignite/internal/network/serialization/FieldDescriptor.java
##########
@@ -44,35 +44,115 @@
*/
private final boolean unshared;
+ private final boolean isPrimitive;
+ private final boolean isRuntimeTypeKnownUpfront;
+
/**
* Accessor for accessing this field.
*/
private final FieldAccessor accessor;
+ /**
+ * Creates a {@link FieldDescriptor} from a local {@link Field}.
+ */
+ public static FieldDescriptor local(Field field, int typeDescriptorId) {
+ return new FieldDescriptor(field, typeDescriptorId);
+ }
+
+ /**
+ * Creates a {@link FieldDescriptor} from a field defined by name, but
having local class.
+ */
+ public static FieldDescriptor local(
+ String fieldName,
+ Class<?> fieldClazz,
+ int typeDescriptorId,
+ boolean unshared,
+ Class<?> declaringClass) {
+ return new FieldDescriptor(
+ fieldName,
+ fieldClazz,
+ typeDescriptorId,
+ unshared,
+ fieldClazz.isPrimitive(),
+ Classes.isRuntimeTypeKnownUpfront(fieldClazz),
+ declaringClass
+ );
+ }
+
+ /**
+ * Creates a {@link FieldDescriptor} from a possibly remote field.
Review comment:
What does 'possibly remote' stands for?
##########
File path:
modules/network/src/main/java/org/apache/ignite/internal/network/serialization/marshal/SchemaMismatchHandler.java
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.network.serialization.marshal;
Review comment:
Do you think this should be an internal feature?
##########
File path:
modules/network/src/main/java/org/apache/ignite/internal/network/serialization/MergedField.java
##########
@@ -0,0 +1,142 @@
+/*
+ * 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.network.serialization;
+
+import java.util.Objects;
+
+/**
+ * Contains information about a field, both from local and remote classes. Any
of the parts (local/remote) might be absent.
+ */
+public class MergedField {
+ private final FieldDescriptor localField;
+ private final FieldDescriptor remoteField;
+
+ /**
+ * Creates a {@link MergedField} for the case when there is only local
part.
+ *
+ * @param localField local part
+ * @return merged field
+ */
+ public static MergedField localOnly(FieldDescriptor localField) {
+ return new MergedField(localField, null);
+ }
+
+ /**
+ * Creates a {@link MergedField} for the case when there is only remote
part.
+ *
+ * @param remoteField remote part
+ * @return merged field
+ */
+ public static MergedField remoteOnly(FieldDescriptor remoteField) {
+ return new MergedField(null, remoteField);
+ }
+
+ /**
+ * Creates a new {@link MergedField}.
+ *
+ * @param localField local part
+ * @param remoteField remote part
+ */
+ public MergedField(FieldDescriptor localField, FieldDescriptor
remoteField) {
+ if (localField == null && remoteField == null) {
+ throw new IllegalArgumentException("Both descriptors are null");
+ }
+ if (localField != null && remoteField != null &&
!localField.name().equals(remoteField.name())) {
Review comment:
all these checks can be assertions because we do check names in the
merger and non-null is also implied by the merger
--
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]
