[ 
https://issues.apache.org/jira/browse/FLINK-5066?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15719898#comment-15719898
 ] 

ASF GitHub Bot commented on FLINK-5066:
---------------------------------------

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.


> prevent LocalInputChannel#getNextBuffer from de-serialising all events when 
> looking for EndOfPartitionEvent only
> ----------------------------------------------------------------------------------------------------------------
>
>                 Key: FLINK-5066
>                 URL: https://issues.apache.org/jira/browse/FLINK-5066
>             Project: Flink
>          Issue Type: Improvement
>          Components: Network
>            Reporter: Nico Kruber
>            Assignee: Nico Kruber
>
> LocalInputChannel#getNextBuffer de-serialises all incoming events on the 
> lookout for an EndOfPartitionEvent.
> Instead, if EventSerializer offered a function to check for an event type 
> only without de-serialising the whole event, we could save some resources.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to