You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ignite.apache.org by sd...@apache.org on 2022/05/06 13:18:07 UTC
[ignite-3] branch main updated: IGNITE-16571 Implement defaultness semantics for GetField when schema changes (#798)
This is an automated email from the ASF dual-hosted git repository.
sdanilov pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/ignite-3.git
The following commit(s) were added to refs/heads/main by this push:
new 9221b5a5d IGNITE-16571 Implement defaultness semantics for GetField when schema changes (#798)
9221b5a5d is described below
commit 9221b5a5d5bed5348cea9585c40f8bbfe29a6a06
Author: Roman Puchkovskiy <ro...@gmail.com>
AuthorDate: Fri May 6 17:18:02 2022 +0400
IGNITE-16571 Implement defaultness semantics for GetField when schema changes (#798)
---
.../network/serialization/ClassDescriptor.java | 90 ++++++++++++++---
.../marshal/UosObjectInputStream.java | 77 ++++++++++-----
...ltUserObjectMarshallerWithSchemaChangeTest.java | 107 ++++++++++++++++++++-
...shallerWithSerializableOverrideStreamsTest.java | 4 +-
4 files changed, 238 insertions(+), 40 deletions(-)
diff --git a/modules/network/src/main/java/org/apache/ignite/internal/network/serialization/ClassDescriptor.java b/modules/network/src/main/java/org/apache/ignite/internal/network/serialization/ClassDescriptor.java
index d146b219c..c108beaee 100644
--- a/modules/network/src/main/java/org/apache/ignite/internal/network/serialization/ClassDescriptor.java
+++ b/modules/network/src/main/java/org/apache/ignite/internal/network/serialization/ClassDescriptor.java
@@ -635,7 +635,7 @@ public class ClassDescriptor implements DeclaredType {
}
/**
- * Return offset into primitive fields data (which has size {@link #primitiveFieldsDataSize()}).
+ * Returns offset into primitive fields data (which has size {@link #primitiveFieldsDataSize()}).
* These are different from the offsets used in the context of {@link sun.misc.Unsafe}.
*
* @param fieldName primitive field name
@@ -649,21 +649,22 @@ public class ClassDescriptor implements DeclaredType {
+ ", but it was used as " + requiredTypeName);
}
- if (primitiveFieldDataOffsets == null) {
- primitiveFieldDataOffsets = primitiveFieldDataOffsetsMap(fields);
- }
+ ensurePrimitiveDataOffsetsMapComputed();
assert primitiveFieldDataOffsets.containsKey(fieldName);
return primitiveFieldDataOffsets.getInt(fieldName);
}
- private FieldDescriptor requiredFieldByName(String fieldName) {
- if (fieldsByName == null) {
- fieldsByName = fieldsByNameMap(fields);
+ private void ensurePrimitiveDataOffsetsMapComputed() {
+ if (primitiveFieldDataOffsets == null) {
+ primitiveFieldDataOffsets = primitiveFieldDataOffsetsMap(fields);
}
+ }
+
+ private FieldDescriptor requiredFieldByName(String fieldName) {
+ FieldDescriptor fieldDesc = fieldByName(fieldName);
- FieldDescriptor fieldDesc = fieldsByName.get(fieldName);
if (fieldDesc == null) {
throw new IllegalStateException("Did not find a field with name " + fieldName);
}
@@ -671,6 +672,15 @@ public class ClassDescriptor implements DeclaredType {
return fieldDesc;
}
+ @Nullable
+ private FieldDescriptor fieldByName(String fieldName) {
+ if (fieldsByName == null) {
+ fieldsByName = fieldsByNameMap(fields);
+ }
+
+ return fieldsByName.get(fieldName);
+ }
+
private static Map<String, FieldDescriptor> fieldsByNameMap(List<FieldDescriptor> fields) {
return fields.stream()
.collect(toUnmodifiableMap(FieldDescriptor::name, Function.identity()));
@@ -690,6 +700,36 @@ public class ClassDescriptor implements DeclaredType {
return Object2IntMaps.unmodifiable(map);
}
+ /**
+ * Returns {@code true} iff the described class has a primitive field with the given name.
+ *
+ * @param fieldName primitive field name
+ * @return {@code true} iff the described class has a primitive field with the given name
+ */
+ private boolean hasPrimitiveField(String fieldName) {
+ FieldDescriptor fieldDesc = fieldByName(fieldName);
+
+ if (fieldDesc == null) {
+ return false;
+ }
+
+ ensurePrimitiveDataOffsetsMapComputed();
+
+ return primitiveFieldDataOffsets.containsKey(fieldName);
+ }
+
+ /**
+ * Returns {@code true} iff the given primitive field does not exist in the remote class corresponding to this descriptor,
+ * but it does exist in the local version of the class.
+ *
+ * @param fieldName primitive field name
+ * @return {@code true} iff the given primitive field does not exist in the remote class corresponding to this descriptor,
+ * but it does exist in the local version of the class
+ */
+ public boolean isPrimitiveFieldAddedLocally(String fieldName) {
+ return !this.hasPrimitiveField(fieldName) && this.local().hasPrimitiveField(fieldName);
+ }
+
/**
* Returns index of a non-primitive (i.e. object) field in the object fields array.
*
@@ -697,9 +737,7 @@ public class ClassDescriptor implements DeclaredType {
* @return index of a non-primitive (i.e. object) field in the object fields array
*/
public int objectFieldIndex(String fieldName) {
- if (objectFieldIndices == null) {
- objectFieldIndices = computeObjectFieldIndices(fields);
- }
+ ensureObjectFieldIndicesComputed();
if (!objectFieldIndices.containsKey(fieldName)) {
throw new IllegalStateException("Did not find an object field with name " + fieldName);
@@ -708,6 +746,12 @@ public class ClassDescriptor implements DeclaredType {
return objectFieldIndices.getInt(fieldName);
}
+ private void ensureObjectFieldIndicesComputed() {
+ if (objectFieldIndices == null) {
+ objectFieldIndices = computeObjectFieldIndices(fields);
+ }
+ }
+
private Object2IntMap<String> computeObjectFieldIndices(List<FieldDescriptor> fields) {
Object2IntMap<String> map = new Object2IntOpenHashMap<>();
@@ -722,6 +766,30 @@ public class ClassDescriptor implements DeclaredType {
return Object2IntMaps.unmodifiable(map);
}
+ /**
+ * Returns @{code true} iff the described class has an object field with the given name.
+ *
+ * @param fieldName object field name
+ * @return @{code true} iff the described class has an object field with the given name
+ */
+ private boolean hasObjectField(String fieldName) {
+ ensureObjectFieldIndicesComputed();
+
+ return objectFieldIndices.containsKey(fieldName);
+ }
+
+ /**
+ * Returns {@code true} iff the given object field does not exist in the remote class corresponding to this descriptor,
+ * but it does exist in the local version of the class.
+ *
+ * @param fieldName object field name
+ * @return {@code true} iff the given object field does not exist in the remote class corresponding to this descriptor,
+ * but it does exist in the local version of the class
+ */
+ public boolean isObjectFieldAddedLocally(String fieldName) {
+ return !this.hasObjectField(fieldName) && this.local().hasObjectField(fieldName);
+ }
+
/**
* Returns the lineage (all the ancestors, from the progenitor (excluding Object) down the line, including this descriptor).
*
diff --git a/modules/network/src/main/java/org/apache/ignite/internal/network/serialization/marshal/UosObjectInputStream.java b/modules/network/src/main/java/org/apache/ignite/internal/network/serialization/marshal/UosObjectInputStream.java
index d7fc2dd2d..e3913c89f 100644
--- a/modules/network/src/main/java/org/apache/ignite/internal/network/serialization/marshal/UosObjectInputStream.java
+++ b/modules/network/src/main/java/org/apache/ignite/internal/network/serialization/marshal/UosObjectInputStream.java
@@ -254,13 +254,13 @@ class UosObjectInputStream extends ObjectInputStream {
class UosGetField extends GetField {
private final DataInput input = UosObjectInputStream.this;
- private final ClassDescriptor descriptor;
+ private final ClassDescriptor remoteDescriptor;
private final byte[] primitiveFieldsData;
private final Object[] objectFieldVals;
private UosGetField(ClassDescriptor currentObjectDescriptor) {
- this.descriptor = currentObjectDescriptor;
+ this.remoteDescriptor = currentObjectDescriptor;
primitiveFieldsData = new byte[currentObjectDescriptor.primitiveFieldsDataSize()];
objectFieldVals = new Object[currentObjectDescriptor.objectFieldsCount()];
@@ -271,81 +271,114 @@ class UosObjectInputStream extends ObjectInputStream {
public ObjectStreamClass getObjectStreamClass() {
// TODO: IGNITE-16572 - make it support schema changes
- return ObjectStreamClass.lookupAny(descriptor.localClass());
+ return ObjectStreamClass.lookupAny(remoteDescriptor.localClass());
}
/** {@inheritDoc} */
@Override
public boolean defaulted(String name) {
- // TODO: IGNITE-16571 - actually take into account whether it's defaulted or not
- return false;
+ return remoteDescriptor.isPrimitiveFieldAddedLocally(name) || remoteDescriptor.isObjectFieldAddedLocally(name);
}
- // TODO: IGNITE-16571 - return default values if the field exists locally but not in the stream being parsed
-
/** {@inheritDoc} */
@Override
- public boolean get(String name, boolean val) throws IOException {
+ public boolean get(String name, boolean defaultValue) throws IOException {
+ if (remoteDescriptor.isPrimitiveFieldAddedLocally(name)) {
+ return defaultValue;
+ }
+
return LittleEndianBits.getBoolean(primitiveFieldsData, primitiveFieldDataOffset(name, boolean.class));
}
/** {@inheritDoc} */
@Override
- public byte get(String name, byte val) throws IOException {
+ public byte get(String name, byte defaultValue) throws IOException {
+ if (remoteDescriptor.isPrimitiveFieldAddedLocally(name)) {
+ return defaultValue;
+ }
+
return primitiveFieldsData[primitiveFieldDataOffset(name, byte.class)];
}
/** {@inheritDoc} */
@Override
- public char get(String name, char val) throws IOException {
+ public char get(String name, char defaultValue) throws IOException {
+ if (remoteDescriptor.isPrimitiveFieldAddedLocally(name)) {
+ return defaultValue;
+ }
+
return LittleEndianBits.getChar(primitiveFieldsData, primitiveFieldDataOffset(name, char.class));
}
/** {@inheritDoc} */
@Override
- public short get(String name, short val) throws IOException {
+ public short get(String name, short defaultValue) throws IOException {
+ if (remoteDescriptor.isPrimitiveFieldAddedLocally(name)) {
+ return defaultValue;
+ }
+
return LittleEndianBits.getShort(primitiveFieldsData, primitiveFieldDataOffset(name, short.class));
}
/** {@inheritDoc} */
@Override
- public int get(String name, int val) throws IOException {
+ public int get(String name, int defaultValue) throws IOException {
+ if (remoteDescriptor.isPrimitiveFieldAddedLocally(name)) {
+ return defaultValue;
+ }
+
return LittleEndianBits.getInt(primitiveFieldsData, primitiveFieldDataOffset(name, int.class));
}
/** {@inheritDoc} */
@Override
- public long get(String name, long val) throws IOException {
+ public long get(String name, long defaultValue) throws IOException {
+ if (remoteDescriptor.isPrimitiveFieldAddedLocally(name)) {
+ return defaultValue;
+ }
+
return LittleEndianBits.getLong(primitiveFieldsData, primitiveFieldDataOffset(name, long.class));
}
/** {@inheritDoc} */
@Override
- public float get(String name, float val) throws IOException {
+ public float get(String name, float defaultValue) throws IOException {
+ if (remoteDescriptor.isPrimitiveFieldAddedLocally(name)) {
+ return defaultValue;
+ }
+
return LittleEndianBits.getFloat(primitiveFieldsData, primitiveFieldDataOffset(name, float.class));
}
/** {@inheritDoc} */
@Override
- public double get(String name, double val) throws IOException {
+ public double get(String name, double defaultValue) throws IOException {
+ if (remoteDescriptor.isPrimitiveFieldAddedLocally(name)) {
+ return defaultValue;
+ }
+
return LittleEndianBits.getDouble(primitiveFieldsData, primitiveFieldDataOffset(name, double.class));
}
/** {@inheritDoc} */
@Override
- public Object get(String name, Object val) throws IOException {
- return objectFieldVals[descriptor.objectFieldIndex(name)];
+ public Object get(String name, Object defaultValue) throws IOException {
+ if (remoteDescriptor.isObjectFieldAddedLocally(name)) {
+ return defaultValue;
+ }
+
+ return objectFieldVals[remoteDescriptor.objectFieldIndex(name)];
}
private int primitiveFieldDataOffset(String fieldName, Class<?> requiredType) {
- return descriptor.primitiveFieldDataOffset(fieldName, requiredType.getName());
+ return remoteDescriptor.primitiveFieldDataOffset(fieldName, requiredType.getName());
}
private void readFields() throws IOException {
- @Nullable BitSet nullsBitSet = NullsBitsetReader.readNullsBitSet(input, descriptor);
+ @Nullable BitSet nullsBitSet = NullsBitsetReader.readNullsBitSet(input, remoteDescriptor);
int objectFieldIndex = 0;
- for (FieldDescriptor fieldDesc : descriptor.fields()) {
+ for (FieldDescriptor fieldDesc : remoteDescriptor.fields()) {
objectFieldIndex = readNext(fieldDesc, objectFieldIndex, nullsBitSet);
}
}
@@ -360,13 +393,13 @@ class UosObjectInputStream extends ObjectInputStream {
}
private void readPrimitive(FieldDescriptor fieldDesc) throws IOException {
- int offset = descriptor.primitiveFieldDataOffset(fieldDesc.name(), fieldDesc.typeName());
+ int offset = remoteDescriptor.primitiveFieldDataOffset(fieldDesc.name(), fieldDesc.typeName());
int length = fieldDesc.primitiveWidthInBytes();
input.readFully(primitiveFieldsData, offset, length);
}
private int readObject(FieldDescriptor fieldDesc, int objectFieldIndex, @Nullable BitSet nullsBitSet) throws IOException {
- if (!StructuredObjectMarshaller.nullWasSkippedWhileWriting(fieldDesc, descriptor, nullsBitSet)) {
+ if (!StructuredObjectMarshaller.nullWasSkippedWhileWriting(fieldDesc, remoteDescriptor, nullsBitSet)) {
Object readObject;
if (fieldDesc.isUnshared()) {
readObject = doReadUnsharedOf(fieldDesc);
diff --git a/modules/network/src/test/java/org/apache/ignite/internal/network/serialization/marshal/DefaultUserObjectMarshallerWithSchemaChangeTest.java b/modules/network/src/test/java/org/apache/ignite/internal/network/serialization/marshal/DefaultUserObjectMarshallerWithSchemaChangeTest.java
index 1ee565cf7..e61dfaef7 100644
--- a/modules/network/src/test/java/org/apache/ignite/internal/network/serialization/marshal/DefaultUserObjectMarshallerWithSchemaChangeTest.java
+++ b/modules/network/src/test/java/org/apache/ignite/internal/network/serialization/marshal/DefaultUserObjectMarshallerWithSchemaChangeTest.java
@@ -38,6 +38,7 @@ import java.io.IOException;
import java.io.InputStream;
import java.io.ObjectInput;
import java.io.ObjectInputStream;
+import java.io.ObjectInputStream.GetField;
import java.io.ObjectOutput;
import java.io.ObjectOutputStream;
import java.io.Serializable;
@@ -69,7 +70,7 @@ import org.mockito.junit.jupiter.MockitoExtension;
* with a class which structure differs from our local version of the class.
*/
@ExtendWith(MockitoExtension.class)
-class DefaultUserObjectMarshallerWithSchemaChangeTest {
+public class DefaultUserObjectMarshallerWithSchemaChangeTest {
private static final byte[] INT_42_BYTES_IN_LITTLE_ENDIAN = {42, 0, 0, 0};
private final ClassDescriptorRegistry localRegistry = new ClassDescriptorRegistry();
@@ -83,6 +84,8 @@ class DefaultUserObjectMarshallerWithSchemaChangeTest {
@Mock
private SchemaMismatchHandler<Object> schemaMismatchHandler;
+ public static GetFieldReader getFieldReader;
+
@SuppressWarnings("unchecked")
@ParameterizedTest
@MethodSource("extraFields")
@@ -217,7 +220,7 @@ class DefaultUserObjectMarshallerWithSchemaChangeTest {
ClassDescriptor remoteDescriptor = remoteRegistry.getRequiredDescriptor(remoteClass);
- ClassDescriptor reconstructedDescriptor = ClassDescriptor.forRemote(
+ ClassDescriptor reconstructedRemoteDescriptor = ClassDescriptor.forRemote(
localDescriptor.localClass(),
remoteDescriptor.descriptorId(),
remoteDescriptor.superClassDescriptor(),
@@ -233,9 +236,9 @@ class DefaultUserObjectMarshallerWithSchemaChangeTest {
CompositeDescriptorRegistry compositeRegistry = new CompositeDescriptorRegistry(
new MapBackedIdIndexedDescriptors(
- Int2ObjectMaps.singleton(reconstructedDescriptor.descriptorId(), reconstructedDescriptor)
+ Int2ObjectMaps.singleton(reconstructedRemoteDescriptor.descriptorId(), reconstructedRemoteDescriptor)
),
- new MapBackedClassIndexedDescriptors(Map.of(reconstructedDescriptor.localClass(), reconstructedDescriptor)),
+ new MapBackedClassIndexedDescriptors(Map.of(reconstructedRemoteDescriptor.localClass(), reconstructedRemoteDescriptor)),
localRegistry
);
@@ -275,6 +278,53 @@ class DefaultUserObjectMarshallerWithSchemaChangeTest {
}
}
+ @ParameterizedTest
+ @MethodSource("extraFieldsForGetField")
+ void getFieldReturnsDefaultValueWhenRemoteClassHasExtraField(ExtraFieldForGetField extraField) throws Exception {
+ Class<?> remoteClass = SerializableWithDefaultedGetField.class;
+ Class<?> localClass = addFieldTo(remoteClass, "addedLocally", extraField.type);
+
+ Object remoteInstance = instantiate(remoteClass);
+
+ getFieldReader = extraField.reader;
+
+ Object unmarshalled = marshalRemotelyAndUnmarshalLocally(remoteInstance, localClass, remoteClass);
+
+ Object valueReadFromGetField = IgniteTestUtils.getFieldValue(unmarshalled, unmarshalled.getClass(), "readValue");
+ assertThat(valueReadFromGetField, is(extraField.expectedValue));
+ }
+
+ private static Stream<Arguments> extraFieldsForGetField() {
+ String fieldName = "addedLocally";
+ return Stream.of(
+ new ExtraFieldForGetField(byte.class, (byte) 10, field -> field.get(fieldName, (byte) 10)),
+ new ExtraFieldForGetField(short.class, (short) 11, field -> field.get(fieldName, (short) 11)),
+ new ExtraFieldForGetField(int.class, 12, field -> field.get(fieldName, 12)),
+ new ExtraFieldForGetField(long.class, (long) 13, field -> field.get(fieldName, (long) 13)),
+ new ExtraFieldForGetField(float.class, (float) 14, field -> field.get(fieldName, (float) 14)),
+ new ExtraFieldForGetField(double.class, (double) 15, field -> field.get(fieldName, (double) 15)),
+ new ExtraFieldForGetField(char.class, 'x', field -> field.get(fieldName, 'x')),
+ new ExtraFieldForGetField(boolean.class, true, field -> field.get(fieldName, true)),
+ new ExtraFieldForGetField(String.class, "Bye", field -> field.get(fieldName, "Bye"))
+ ).map(Arguments::of);
+ }
+
+ @ParameterizedTest
+ @MethodSource("extraFields")
+ void getFieldDefaultedReturnsTrueForFieldsAddedLocally(ExtraField extraField) throws Exception {
+ Class<?> remoteClass = SerializableWithDefaultedGetField.class;
+ Class<?> localClass = addFieldTo(remoteClass, "addedLocally", extraField.type);
+
+ Object remoteInstance = instantiate(remoteClass);
+
+ getFieldReader = getField -> getField.defaulted("addedLocally");
+
+ Object unmarshalled = marshalRemotelyAndUnmarshalLocally(remoteInstance, localClass, remoteClass);
+
+ Object valueReadFromGetField = IgniteTestUtils.getFieldValue(unmarshalled, unmarshalled.getClass(), "readValue");
+ assertThat(valueReadFromGetField, is(true));
+ }
+
@Test
void removalOfExternalizableInterfaceCausesUnfilledDeserializationResult() throws Exception {
Object unmarshalled = marshalExternalizableUnmarshalNonExternalizableBasedOn(ExternalizationReady.class);
@@ -534,6 +584,55 @@ class DefaultUserObjectMarshallerWithSchemaChangeTest {
}
}
+ private static class SerializableWithDefaultedGetField implements Serializable {
+ Object readValue;
+
+ private void writeObject(ObjectOutputStream stream) throws IOException {
+ stream.putFields();
+ stream.writeFields();;
+ }
+
+ private void readObject(ObjectInputStream stream) throws IOException, ClassNotFoundException {
+ GetField getField = stream.readFields();
+ readValue = getFieldReader.read(getField);
+ }
+ }
+
+ /**
+ * Reads a value from {@link GetField}.
+ */
+ public interface GetFieldReader {
+ /**
+ * Reads a value from the given {@link GetField}.
+ *
+ * @param getField {@link GetField} instance
+ * @return a value extracted from GetField
+ * @throws IOException if something goes wrong
+ */
+ Object read(GetField getField) throws IOException;
+ }
+
+ private static class ExtraFieldForGetField {
+ private final Class<?> type;
+ private final Object expectedValue;
+ private final GetFieldReader reader;
+
+ private ExtraFieldForGetField(Class<?> type, Object expectedValue, GetFieldReader reader) {
+ this.type = type;
+ this.expectedValue = expectedValue;
+ this.reader = reader;
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public String toString() {
+ return "ExtraFieldForGetField{"
+ + "type=" + type
+ + ", value=" + expectedValue
+ + '}';
+ }
+ }
+
private static class ExternalizationReady {
private int value;
diff --git a/modules/network/src/test/java/org/apache/ignite/internal/network/serialization/marshal/DefaultUserObjectMarshallerWithSerializableOverrideStreamsTest.java b/modules/network/src/test/java/org/apache/ignite/internal/network/serialization/marshal/DefaultUserObjectMarshallerWithSerializableOverrideStreamsTest.java
index a0ee76505..e300c47bd 100644
--- a/modules/network/src/test/java/org/apache/ignite/internal/network/serialization/marshal/DefaultUserObjectMarshallerWithSerializableOverrideStreamsTest.java
+++ b/modules/network/src/test/java/org/apache/ignite/internal/network/serialization/marshal/DefaultUserObjectMarshallerWithSerializableOverrideStreamsTest.java
@@ -408,9 +408,7 @@ class DefaultUserObjectMarshallerWithSerializableOverrideStreamsTest {
}
@Test
- void getFieldAlwaysReturnsFalseForDefaulted() {
- // TODO: IGNITE-16571 - test that defaulted() works as intended when it's ready
-
+ void getFieldDefaultedReturnsFalseIfFieldExistsBeforeAndAfterSerialization() {
fieldFiller = (getField, target) -> {
assertFalse(getField.defaulted("byteVal"));
};