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

Add an Option to Deactivate Kryo Fallback for Serializers

    Details

      Description

      Some users want to avoid that Flink's serializers use Kryo, as it can easily become a hotspot in serialization.

      For those users, it would help if there is a flag to "deactive generic types". Those users could then see where types are used that default to Kryo and change these types (make them PoJos, Value types, or write custom serializers).

      There are two ways to approach that:

      1. (Simple) Make GenericTypeInfo threw an exception whenever it would create a Kryo Serializer (when the respective flag is set in the ExecutionConfig)

      2. Have a static flag on the TypeExtractor to throw an exception whenever it would create a GenericTypeInfo. This approach has the downside of introducing some static configuration to the TypeExtractor, but may be more helpful because it throws exceptions in the programs at points where the types are used (not where the serializers are created, which may be much later).

        Issue Links

          Activity

          Hide
          StephanEwen Stephan Ewen added a comment -

          Fixed in 0f99aae1e1f8b693c2ba79a061046bc042113f0b with small followups in d498cbedfda7c5ebabbc5f8203d7518926ede423

          Show
          StephanEwen Stephan Ewen added a comment - Fixed in 0f99aae1e1f8b693c2ba79a061046bc042113f0b with small followups in d498cbedfda7c5ebabbc5f8203d7518926ede423
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

          Github user StephanEwen commented on the issue:

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

          The change looks good, merging this...

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3373 The change looks good, merging this...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jinmingjian commented on the issue:

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

          @StephanEwen Just my coding habit. Correction done.

          And very appreciated for your review. I am open for more contribution! :tada:

          Show
          githubbot ASF GitHub Bot added a comment - Github user jinmingjian commented on the issue: https://github.com/apache/flink/pull/3373 @StephanEwen Just my coding habit. Correction done. And very appreciated for your review. I am open for more contribution! :tada:
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

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

          If you can remove the comment, that would be great. Otherwise we can merge this...

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3373 If you can remove the comment, that would be great. Otherwise we can merge this...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3373#discussion_r102753321

          — Diff: flink-core/src/main/java/org/apache/flink/api/common/ExecutionConfig.java —
          @@ -109,6 +109,8 @@

          private boolean forceKryo = false;

          + private boolean disableGenericTypes = false;//ISSUE FLINK-5692
          — End diff –

          We usually do not add the issues as comments to the code. The git history stores that (commits have the issue number).

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3373#discussion_r102753321 — Diff: flink-core/src/main/java/org/apache/flink/api/common/ExecutionConfig.java — @@ -109,6 +109,8 @@ private boolean forceKryo = false; + private boolean disableGenericTypes = false;//ISSUE FLINK-5692 — End diff – We usually do not add the issues as comments to the code. The git history stores that (commits have the issue number).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jinmingjian commented on the issue:

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

          revision done. "disableGenericTypes" makes good sense from the view of docs (although "generic types" tends to be overused).

          Show
          githubbot ASF GitHub Bot added a comment - Github user jinmingjian commented on the issue: https://github.com/apache/flink/pull/3373 revision done. "disableGenericTypes" makes good sense from the view of docs (although "generic types" tends to be overused).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3373#discussion_r102429969

          — Diff: flink-core/src/test/java/org/apache/flink/api/common/ExecutionConfigTest.java —
          @@ -64,4 +68,15 @@ public void testConfigurationOfParallelism()

          { assertEquals(parallelism, config.getParallelism()); }

          + @Test(expected = UnsupportedOperationException.class)
          — End diff –

          I think this test would also pass of line 75 already throws the exception.

          I personally discourage the `@Test(expected = )` pattern completely, as you have no check which statement actually throws the exception.

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3373#discussion_r102429969 — Diff: flink-core/src/test/java/org/apache/flink/api/common/ExecutionConfigTest.java — @@ -64,4 +68,15 @@ public void testConfigurationOfParallelism() { assertEquals(parallelism, config.getParallelism()); } + @Test(expected = UnsupportedOperationException.class) — End diff – I think this test would also pass of line 75 already throws the exception. I personally discourage the `@Test(expected = )` pattern completely, as you have no check which statement actually throws the exception.
          Hide
          jinmingjian Jin Mingjian added a comment -

          Stephan Ewen, I use "forceCustomSerializerCheck" to follow the existed option naming style. Let me know if you think some other is better.

          Show
          jinmingjian Jin Mingjian added a comment - Stephan Ewen , I use "forceCustomSerializerCheck" to follow the existed option naming style. Let me know if you think some other is better.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user jinmingjian opened a pull request:

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

          FLINK-5692 [config] Add an Option to Deactivate Kryo Fallback for Serializers

          Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
          If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide](http://flink.apache.org/how-to-contribute.html).
          In addition to going through the list, please provide a meaningful description of your changes.

          • [x] General
          • The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
          • The pull request addresses only one issue
          • Each commit in the PR has a meaningful commit message (including the JIRA id)
          • [x] Documentation
          • Documentation has been added for new functionality
          • Old documentation affected by the pull request has been updated
          • JavaDoc for public methods has been added
          • [x] Tests & Build
          • Functionality added by the pull request is covered by tests
          • `mvn clean verify` has been executed successfully locally or a Travis build has passed

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

          $ git pull https://github.com/jinmingjian/flink master

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

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


          commit 1ff46e53efa2094ac6881b1ca014bf7752277ff2
          Author: Jin Mingjian <jin.phd@gmail.com>
          Date: 2017-02-21T03:57:21Z

          FLINK-5692 [config] Add an Option to Deactivate Kryo Fallback for Serializers


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user jinmingjian opened a pull request: https://github.com/apache/flink/pull/3373 FLINK-5692 [config] Add an Option to Deactivate Kryo Fallback for Serializers Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration. If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide] ( http://flink.apache.org/how-to-contribute.html ). In addition to going through the list, please provide a meaningful description of your changes. [x] General The pull request references the related JIRA issue (" [FLINK-XXX] Jira title text") The pull request addresses only one issue Each commit in the PR has a meaningful commit message (including the JIRA id) [x] Documentation Documentation has been added for new functionality Old documentation affected by the pull request has been updated JavaDoc for public methods has been added [x] Tests & Build Functionality added by the pull request is covered by tests `mvn clean verify` has been executed successfully locally or a Travis build has passed You can merge this pull request into a Git repository by running: $ git pull https://github.com/jinmingjian/flink master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3373.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 #3373 commit 1ff46e53efa2094ac6881b1ca014bf7752277ff2 Author: Jin Mingjian <jin.phd@gmail.com> Date: 2017-02-21T03:57:21Z FLINK-5692 [config] Add an Option to Deactivate Kryo Fallback for Serializers
          Hide
          StephanEwen Stephan Ewen added a comment -

          Jin Mingjian That right, #1 approach would work for a start

          Show
          StephanEwen Stephan Ewen added a comment - Jin Mingjian That right, #1 approach would work for a start
          Hide
          jinmingjian Jin Mingjian added a comment -

          Stephan Ewen If my understanding is not wrong, the goal of this option is just to let users to know what the types should be provided with custom serializers but actually not when their application running. Then they can provide it explicitly rather than silent Kryo fallback by Flink in default.

          For this goal, way #1 is enough.

          Show
          jinmingjian Jin Mingjian added a comment - Stephan Ewen If my understanding is not wrong, the goal of this option is just to let users to know what the types should be provided with custom serializers but actually not when their application running. Then they can provide it explicitly rather than silent Kryo fallback by Flink in default. For this goal, way #1 is enough.
          Hide
          jinmingjian Jin Mingjian added a comment -

          Interesting to see a 'starter' issue with the 'Major' priority I would like to work out a PR as my entrance contribution to Flink.

          Show
          jinmingjian Jin Mingjian added a comment - Interesting to see a 'starter' issue with the 'Major' priority I would like to work out a PR as my entrance contribution to Flink.

            People

            • Assignee:
              jinmingjian Jin Mingjian
              Reporter:
              StephanEwen Stephan Ewen
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development