[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user mxm commented on the issue: https://github.com/apache/flink/pull/2623 Sure, I'll take care of it. I saw you giving a +1 and didn't see an issue myself. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/2623 @mxm @chermenin Can we please do a followup to this, given that it is already merged? The tests in `GroupReduceITCase` need to be replaced by proper targeted unit tests. Sorry for being super strict, but our build times are already exploding, and it is exactly cases like this one (adding distributed runtime Integration Tests to test a serializer) that contribute a lot to that. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user mxm commented on the issue: https://github.com/apache/flink/pull/2623 Thank you! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user chermenin commented on the issue: https://github.com/apache/flink/pull/2623 Yep, fixed. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user mxm commented on the issue: https://github.com/apache/flink/pull/2623 The build fails during the Maven Rat plugin license check. Could you fix the build and rebase to the latest master? I think we can merge then. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user chermenin commented on the issue: https://github.com/apache/flink/pull/2623 @StephanEwen What about merging this PR? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/2623 I like this approach. One minor question, otherwise +1 to go here --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user chermenin commented on the issue: https://github.com/apache/flink/pull/2623 I exclude Kryo as a dependency from Chill and add simple test to check Java collections. Tests added to `flink-tests` because it is depends on `flink-runtime` (where used Chill). --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/2623 @chermenin Do you plan to continue with this pull request? Would be great if you I would suggest to adjust it in the following way: - Update the Chill dependency (validate if they change any existing serializer) - Exclude Kryo version 2 and version 3 from the Chill dependency. That way, Flink's Kryo version should be used by Chill as well. Also, testing the serializers via a "GroupReduce" integration test seems heavy and unspecific. Can you change this to a serializer Unit test? Have a look at the Kryo Serializer unit tests for an example. Do you think that would work? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/2623 Okay, in detail it seems that compatibility is not maintained for "Unsafe based I/O", but maintained for standard I/O. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/2623 As far as I read it, Kryo 3.x is not strictly serialization compatible with 2.x, hence the major version number bump. If the interfaces are still stable, then it should be fine to bump the chill dependency version, exclude any kryo dependency, and add our own 2.x kryo dependency. I would prefer that approach. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user rmetzger commented on the issue: https://github.com/apache/flink/pull/2623 This change adds Kryo-shaded to our dependency tree: ``` [INFO] | +- com.twitter:chill_2.10:jar:0.8.1:compile [INFO] | | +- com.twitter:chill-java:jar:0.8.1:compile [INFO] | | \- com.esotericsoftware:kryo-shaded:jar:3.0.3:compile [INFO] | | \- com.esotericsoftware:minlog:jar:1.3.0:compile ``` I suspect that maven is not recognizing this because chill seems to depend on `kryo-shaded`. Apparently, `kryo-shaded` has a shaded ASM version included, but it is not relocating the regular Kryo classes. So we'll end up having two Kryo versions in our classpath. So if we want to upgrade Kryo, we need to do it explicitly, to avoid having two Kryo versions in our classpath. Another issue we need to consider is the serialization compatibility. Savepoints in Flink could potentially contain data serialized with Kryo 2.24. If we want to provide savepoint compatibility between Flink 1.1 and 1.2, we need to consider that. According to the Kryo documentation, 2.24 to 3.0.0 is serialization compatible (I hope the same holds true for chill): https://github.com/EsotericSoftware/kryo/blob/master/CHANGES.md#2240---300-2014-10-04 I would like to hear @StephanEwen and @uce's opinion on this. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user fhueske commented on the issue: https://github.com/apache/flink/pull/2623 Bumping the version of serializers is a sensitive issue in Flink. If the serialization format changes, savepoints of previous versions can no longer be used. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user chermenin commented on the issue: https://github.com/apache/flink/pull/2623 @rmetzger If I'm not mistaken Kryo 2.24 and 3.0 versions is compatible each other at the level of standard IO serialization. And if it possible we will be able to a little wait for Chill 0.7.5 version (with needed fixes and Kryo 2.x) or migrate Flink to Kryo 3.x version. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user rmetzger commented on the issue: https://github.com/apache/flink/pull/2623 Does this change have any implications on our Kryo version, since Chill since 0.8.0 uses Kryo3 ? https://github.com/twitter/chill/releases/tag/0.8.0 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2623: [FLINK-2608] Updated Twitter Chill version.
Github user zentol commented on the issue: https://github.com/apache/flink/pull/2623 Could you include a simple test to verify that this does in fact fix the issue? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
