flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] pnowojski commented on a change in pull request #6705: [FLINK-10356][network] add sanity checks to SpillingAdaptiveSpanningRecordDeserializer
Date Fri, 01 Mar 2019 14:20:20 GMT
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:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message