Github user uce commented on the issue:
https://github.com/apache/flink/pull/2806
Very good changes in general. The following should be addressed before
merging though:
- Add tests to `EventSerializerTest` with all currently available events
(including `OTHER_EVENT` instances).
- As a special case it's important to make sure that the ByteBuffer
position indexes are not affected by doing a `isEvent` check, because a
following deserialization will fail otherwise. The following example fails
because of this:
```java
ByteBuffer serializedEvent =
EventSerializer.toSerializedEvent(EndOfPartitionEvent.INSTANCE);
assertTrue(EventSerializer.isEvent(serializedEvent,
EndOfPartitionEvent.class, cl));
EndOfPartitionEvent event = (EndOfPartitionEvent)
EventSerializer.fromSerializedEvent(serializedEvent, cl);
```
For `Buffer` instances it works, because `Buffer#getNioBuffer()` always
wraps the memory in a new `ByteBuffer` instance. For the tests, this can be
covered by serializing an event, doing the `isEvent` check on the serialized
buffer and then trying to deserialize the same serialized event buffer (like
above).
- The check is not used in `LocalInputChannel` any more, but only in
`PartitionRequestQueue`. Let's move it there.
- In general the EventSerializer interface could be made a little less
overloaded by only exposing one variant (Buffer/ByteBuffer). That's indepedent
of this issue though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
