pnowojski 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_r261617990
##########
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:
I don't want you to fix other places, but don't make it worse/don't repeat
the same mistake. Imagine that this test is flaky on travis and it failed there
once with unknown seed and now you have to reproduce this flaky test that fails
only 1 out of 2^64 times (`long` seed).
Why fixed seed is bad in this case? You are not touching the content in any
way, but just relaying on the record length (right?), so fixed seed shouldn't
matter.
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
