Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-5066

add more efficient isEvent check to EventSerializer

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0
    • Component/s: Network
    • Labels:
      None

      Description

      LocalInputChannel#getNextBuffer de-serialises all incoming events on the lookout for an EndOfPartitionEvent.
      Some buffer code de-serialises all incoming events on the lookout for an EndOfPartitionEvent
      (now applies to PartitionRequestQueue#isEndOfPartitionEvent()).

      Instead, if EventSerializer offered a function to check for an event type only without de-serialising the whole event, we could save some resources.

        Issue Links

          Activity

          Hide
          uce Ufuk Celebi added a comment -

          Implemented in 9457465 (master).

          Show
          uce Ufuk Celebi added a comment - Implemented in 9457465 (master).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/2806

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/2806
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user uce commented on the issue:

          https://github.com/apache/flink/pull/2806

          Thanks for addressing the comments, Nico. Going to merge this with the next batch.

          Show
          githubbot ASF GitHub Bot added a comment - Github user uce commented on the issue: https://github.com/apache/flink/pull/2806 Thanks for addressing the comments, Nico. Going to merge this with the next batch.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user NicoK commented on the issue:

          https://github.com/apache/flink/pull/2806

          done, and yes, the code is now not relevant for `LocalInputChannel` anymore but for `PartitionRequestQueue` instead

          Show
          githubbot ASF GitHub Bot added a comment - Github user NicoK commented on the issue: https://github.com/apache/flink/pull/2806 done, and yes, the code is now not relevant for `LocalInputChannel` anymore but for `PartitionRequestQueue` instead
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.
          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user NicoK opened a pull request:

          https://github.com/apache/flink/pull/2806

          FLINK-5066 Prevent LocalInputChannel#getNextBuffer from de-serialising all events when looking for EndOfPartitionEvent only

          LocalInputChannel#getNextBuffer de-serialises all incoming events on the lookout for an EndOfPartitionEvent.

          Instead, add EventSerializer.isEvent so that LocalInputChannel can peak into event buffers without de-serialising the whole event, we could save some resources. This is a generic isEvent implementation for any event that fromSerializedEvent currently supports. Alternatively, a specialised method could be implemented looking for EndOfPartitionEvent only, if desired.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/NicoK/flink FLINK-5066

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/2806.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #2806


          commit bf394e8f9ab18991f29942f3017ecc04643ce224
          Author: Nico Kruber <nico@data-artisans.com>
          Date: 2016-11-14T19:02:54Z

          FLINK-5066 add EventSerializer.isEvent so that LocalInputChannel can peak into event buffers


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user NicoK opened a pull request: https://github.com/apache/flink/pull/2806 FLINK-5066 Prevent LocalInputChannel#getNextBuffer from de-serialising all events when looking for EndOfPartitionEvent only LocalInputChannel#getNextBuffer de-serialises all incoming events on the lookout for an EndOfPartitionEvent. Instead, add EventSerializer.isEvent so that LocalInputChannel can peak into event buffers without de-serialising the whole event, we could save some resources. This is a generic isEvent implementation for any event that fromSerializedEvent currently supports. Alternatively, a specialised method could be implemented looking for EndOfPartitionEvent only, if desired. You can merge this pull request into a Git repository by running: $ git pull https://github.com/NicoK/flink FLINK-5066 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/2806.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2806 commit bf394e8f9ab18991f29942f3017ecc04643ce224 Author: Nico Kruber <nico@data-artisans.com> Date: 2016-11-14T19:02:54Z FLINK-5066 add EventSerializer.isEvent so that LocalInputChannel can peak into event buffers

            People

            • Assignee:
              NicoK Nico Kruber
              Reporter:
              NicoK Nico Kruber
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development