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

CompatibilityResult should contain a notCompatible() option

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.3.0, 1.4.0
    • Fix Version/s: 1.3.0, 1.4.0
    • Labels:
      None

      Description

      The CompatibilityResult allows a TypeSerializer to specify whether it is compatible based on the given TypeSerializerConfigSnapshot.

      As it stands the only options are compatible and requiresMigration. We should allow serializers to also notify the system of an incompatibility which should then fail the job.

      This would for example be required when a serializer provides an upgrade path version1 -> version2 -> version3, but not directly from version1 -> version3. Currently, the serializer would either have to contain logic to upgrade from every single previous version or simply throw an exception on it's own.

        Issue Links

          Activity

          Hide
          StephanEwen Stephan Ewen added a comment -

          +1, incompatibility is always possible and should lead to a graceful failure with a good error message

          Show
          StephanEwen Stephan Ewen added a comment - +1, incompatibility is always possible and should lead to a graceful failure with a good error message
          Hide
          tzulitai Tzu-Li (Gordon) Tai added a comment - - edited

          Discussed this with Chesnay Schepler offline.

          An incompatibility of serializers should simply imply a state migration, hence there should be no need of an explicit notCompatible option.
          In the case of version 1 directly to version 3, state migration would be performed as follows:

          • deserialize old state with v1 serializer to objects
          • write with serializer v3.

          There is no need for the v2 serializer. The only case where the job would fail would be if the v1 serializer can no longer be found.

          On the other hand, we do propose to add a CompatibilityResult.requiresMigration() method without a TypeDeserializer parameter to make it more explicit that the serializer can choose not to provide a convert deserializer for migration, if it cannot produce one (in which case we just try to use the old serializer).

          Show
          tzulitai Tzu-Li (Gordon) Tai added a comment - - edited Discussed this with Chesnay Schepler offline. An incompatibility of serializers should simply imply a state migration, hence there should be no need of an explicit notCompatible option. In the case of version 1 directly to version 3, state migration would be performed as follows: deserialize old state with v1 serializer to objects write with serializer v3. There is no need for the v2 serializer. The only case where the job would fail would be if the v1 serializer can no longer be found. On the other hand, we do propose to add a CompatibilityResult.requiresMigration() method without a TypeDeserializer parameter to make it more explicit that the serializer can choose not to provide a convert deserializer for migration, if it cannot produce one (in which case we just try to use the old serializer).
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user tzulitai opened a pull request:

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

          FLINK-6554 [core] Make CompatibilityResult options more explicitly defined

          Previously, if a serializer determines that state migration needs to be
          performed but could not provide a fallback convert deserializer, it
          would use `CompatibilityResult.requiresMigration(null)`.

          This commit makes this option more explicit by having a
          `CompatibilityResult.requiresMigration()` option that takes no parameters.
          This should improve how the user perceives the API without having to rely on
          the Javadoc that it is allowed to have no fallback convert deserializer.

          Consequently, when using `CompatibilityResult.requiresMigration(TypeDeserializer)`, the provided argument cannot be null.

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

          $ git pull https://github.com/tzulitai/flink FLINK-6554

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

          https://github.com/apache/flink/pull/3886.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 #3886


          commit 444329f10f7272ad9964088720c3a8ffb67d30ca
          Author: Tzu-Li (Gordon) Tai <tzulitai@apache.org>
          Date: 2017-05-12T13:00:51Z

          FLINK-6554 [core] Make CompatibilityResult options more explicitly defined

          Previously, if a serializer determines that state migration needs to be
          performed but could not provide a fallback convert deserializer, it
          would use CompatibilityResult.requiresMigration(null).

          This commit makes this option more explicit by having a
          CompatibilityResult.requiresMigration() option that takes no parameters.
          This improves how the user perceives the API without having to rely on
          the Javadoc that it is allowed to have no fallback convert deserializer.

          Consequently, when using
          CompatibilityResult.requiresMigration(TypeDeserializer), the provided
          argument cannot be null.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user tzulitai opened a pull request: https://github.com/apache/flink/pull/3886 FLINK-6554 [core] Make CompatibilityResult options more explicitly defined Previously, if a serializer determines that state migration needs to be performed but could not provide a fallback convert deserializer, it would use `CompatibilityResult.requiresMigration(null)`. This commit makes this option more explicit by having a `CompatibilityResult.requiresMigration()` option that takes no parameters. This should improve how the user perceives the API without having to rely on the Javadoc that it is allowed to have no fallback convert deserializer. Consequently, when using `CompatibilityResult.requiresMigration(TypeDeserializer)`, the provided argument cannot be null. You can merge this pull request into a Git repository by running: $ git pull https://github.com/tzulitai/flink FLINK-6554 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3886.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 #3886 commit 444329f10f7272ad9964088720c3a8ffb67d30ca Author: Tzu-Li (Gordon) Tai <tzulitai@apache.org> Date: 2017-05-12T13:00:51Z FLINK-6554 [core] Make CompatibilityResult options more explicitly defined Previously, if a serializer determines that state migration needs to be performed but could not provide a fallback convert deserializer, it would use CompatibilityResult.requiresMigration(null). This commit makes this option more explicit by having a CompatibilityResult.requiresMigration() option that takes no parameters. This improves how the user perceives the API without having to rely on the Javadoc that it is allowed to have no fallback convert deserializer. Consequently, when using CompatibilityResult.requiresMigration(TypeDeserializer), the provided argument cannot be null.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StefanRRichter commented on the issue:

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

          LGTM +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/3886 LGTM +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3886
          Hide
          tzulitai Tzu-Li (Gordon) Tai added a comment -

          Fixed for master via 947c44e862396baa95e74cbdc50a4c7cd3befe9b.
          Fixed for 1.3 via 4eebf21e9e97719d2a14e8530dcea361de384d64.

          Show
          tzulitai Tzu-Li (Gordon) Tai added a comment - Fixed for master via 947c44e862396baa95e74cbdc50a4c7cd3befe9b. Fixed for 1.3 via 4eebf21e9e97719d2a14e8530dcea361de384d64.

            People

            • Assignee:
              tzulitai Tzu-Li (Gordon) Tai
              Reporter:
              Zentol Chesnay Schepler
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development