[GitHub] flink issue #2806: [FLINK-5066] Prevent LocalInputChannel#getNextBuffer from...

[uce <gi...@git.apache.org>]

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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---