NicoK commented on a change in pull request #6705: [FLINK-10356][network] add
sanity checks to SpillingAdaptiveSpanningRecordDeserializer
URL: https://github.com/apache/flink/pull/6705#discussion_r252367679
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/api/serialization/SpanningRecordSerializationTest.java
##########
@@ -105,11 +119,235 @@ public void testHandleMixedLargeRecords() throws
Exception {
testSerializationRoundTrip(originalRecords, segmentSize);
}
+ /**
+ * Non-spanning, deserialization reads one byte too many and succeeds -
failure report comes
+ * from an additional check in {@link
SpillingAdaptiveSpanningRecordDeserializer}.
+ */
+ @Test
+ public void testHandleDeserializingTooMuchNonSpanning1() throws
Exception {
+ testHandleWrongDeserialization(
+ DeserializingTooMuch.getValue(),
+ 32 * 1024);
+ }
+
+ /**
+ * Non-spanning, deserialization reads one byte too many and fails.
+ */
+ @Test
+ public void testHandleDeserializingTooMuchNonSpanning2() throws
Exception {
+ testHandleWrongDeserialization(
+ DeserializingTooMuch.getValue(),
+ (serializedLength) -> serializedLength,
+ isA(IndexOutOfBoundsException.class));
+ }
+
+ /**
+ * Spanning, deserialization reads one byte too many and fails.
+ */
+ @Test
+ public void testHandleDeserializingTooMuchSpanning1() throws Exception {
+ testHandleWrongDeserialization(
+ DeserializingTooMuch.getValue(),
+ (serializedLength) -> serializedLength - 1,
+ isA(EOFException.class));
+ }
+
+ /**
+ * Spanning, deserialization reads one byte too many and fails.
+ */
+ @Test
+ public void testHandleDeserializingTooMuchSpanning2() throws Exception {
+ testHandleWrongDeserialization(
+ DeserializingTooMuch.getValue(),
+ (serializedLength) -> 1,
+ isA(EOFException.class));
+ }
+
+ /**
+ * Spanning, spilling, deserialization reads one byte too many.
+ */
+ @Test
+ public void testHandleDeserializingTooMuchSpanningLargeRecord() throws
Exception {
+ testHandleWrongDeserialization(
+ LargeObjectTypeDeserializingTooMuch.getRandom(),
+ 32 * 1024,
+ isA(EOFException.class));
+ }
+
+ /**
+ * Non-spanning, deserialization forgets to read one byte - failure
report comes from an
+ * additional check in {@link
SpillingAdaptiveSpanningRecordDeserializer}.
+ */
+ @Test
+ public void testHandleDeserializingNotEnoughNonSpanning() throws
Exception {
+ testHandleWrongDeserialization(
+ DeserializingNotEnough.getValue(),
+ 32 * 1024);
+ }
+
+ /**
+ * Spanning, deserialization forgets to read one byte - failure report
comes from an additional
+ * check in {@link SpillingAdaptiveSpanningRecordDeserializer}.
+ */
+ @Test
+ public void testHandleDeserializingNotEnoughSpanning1() throws
Exception {
+ testHandleWrongDeserialization(
+ DeserializingNotEnough.getValue(),
+ (serializedLength) -> serializedLength - 1);
+ }
+
+ /**
+ * Spanning, serialization length is 17 (including headers),
deserialization forgets to read one
+ * byte - failure report comes from an additional check in {@link
SpillingAdaptiveSpanningRecordDeserializer}.
+ */
+ @Test
+ public void testHandleDeserializingNotEnoughSpanning2() throws
Exception {
+ testHandleWrongDeserialization(
+ DeserializingNotEnough.getValue(),
+ 1);
+ }
+
+ /**
+ * Spanning, spilling, deserialization forgets to read one byte -
failure report comes from an
+ * additional check in {@link
SpillingAdaptiveSpanningRecordDeserializer}.
+ */
+ @Test
+ public void testHandleDeserializingNotEnoughSpanningLargeRecord()
throws Exception {
+ testHandleWrongDeserialization(
+ LargeObjectTypeDeserializingNotEnough.getRandom(),
+ 32 * 1024);
+ }
+
+ private void testHandleWrongDeserialization(
+ WrongDeserializationValue testValue,
+ IntFunction<Integer> segmentSizeProvider,
+ Matcher<? extends Throwable> expectedCause) throws
Exception {
+ expectedException.expectCause(expectedCause);
+ testHandleWrongDeserialization(testValue, segmentSizeProvider);
+ }
+
+ private void testHandleWrongDeserialization(
+ WrongDeserializationValue testValue,
+ @SuppressWarnings("SameParameterValue") int segmentSize,
+ Matcher<? extends Throwable> expectedCause) throws
Exception {
+ expectedException.expectCause(expectedCause);
+ testHandleWrongDeserialization(testValue, segmentSize);
+ }
+
+ private void testHandleWrongDeserialization(
+ WrongDeserializationValue testValue,
+ IntFunction<Integer> segmentSizeProvider) throws
Exception {
+ int serializedBytes = getSerializedBytes(testValue);
+ int segmentSize = segmentSizeProvider.apply(serializedBytes);
+ testHandleWrongDeserialization(testValue, segmentSize);
+ }
+
+ /**
+ * Executes the de/serialization round trip test with a serializer that
consumes more or less
+ * bytes than what it writes and verifies the expected exception is
thrown.
+ */
+ private void testHandleWrongDeserialization(
+ WrongDeserializationValue testValue,
+ int segmentSize) throws Exception {
+ List<IOReadableWritable> originalRecords = new ArrayList<>(1);
+ originalRecords.add(testValue);
+
+ expectedException.expect(IOException.class);
+ if (testValue instanceof DeserializingTooMuch) {
+ expectedException.expectMessage(" -1 remaining unread
byte");
+ } else if (testValue instanceof DeserializingNotEnough) {
+ expectedException.expectMessage(" 1 remaining unread
byte");
+ } else {
+ fail("Invalid test value: " + testValue);
+ }
+
+ testSerializationRoundTrip(originalRecords, segmentSize);
+ }
+
+ /**
+ * Retrieves the number of bytes the serialized representation of the
given value takes (by
+ * doing the serialization).
+ */
+ private int getSerializedBytes(IOReadableWritable testValue) throws
IOException {
+ SpanningRecordSerializer<IOReadableWritable> serializer = new
SpanningRecordSerializer<>();
+ serializer.serializeRecord(testValue);
+ BufferBuilder bufferBuilder = createBufferBuilder();
+ assertTrue("Incorrect test setup: buffer not big enough to
contain test value",
+
serializer.copyToBufferBuilder(bufferBuilder).isFullRecord());
+ int writtenBytes = bufferBuilder.finish();
+ bufferBuilder.createBufferConsumer().close();
+ return writtenBytes;
+ }
+
+ /**
+ * Large object that tries to deserialize an additional byte that it
has not written during
+ * serialization.
+ */
+ public static class LargeObjectTypeDeserializingTooMuch extends
LargeObjectType implements DeserializingTooMuch {
+
+ @SuppressWarnings("WeakerAccess")
+ public LargeObjectTypeDeserializingTooMuch() {
+ }
+
+ @SuppressWarnings("WeakerAccess")
+ public LargeObjectTypeDeserializingTooMuch(int len) {
+ super(len);
+ }
+
+ @Override
+ public void read(DataInputView in) throws IOException {
+ super.read(in);
+ in.readUnsignedByte(); // not written by write()
+ }
+
+ @Override
+ public LargeObjectTypeDeserializingTooMuch getRandom(Random
rnd) {
+ int length = super.getRandom(rnd).length();
+ return new LargeObjectTypeDeserializingTooMuch(length);
+ }
+
+ static LargeObjectTypeDeserializingTooMuch getRandom() {
+ return (new
LargeObjectTypeDeserializingTooMuch()).getRandom(new Random());
Review comment:
Using a fixed seed is not preferable: you'd always test the same record(s)
while otherwise each test would test something else and you get better test
coverage over time. (I don't know why
`org.apache.flink.testutils.serialization.types.Util` is using that pattern
though)
I'll have a quick look on whether it is possible to print something generic
(the seed) in case of unit test failure - the latter part can be done manually
when trying to fix an error.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
