I don't know about the specifics of the kryo 2.x vs 5.x issues. The discussion around FLIP-317 [1] seems to have stalled here. But Java 17 is still considered an experimental feature as expressed in Flink's roadmap [2]. So, it should be fine to have it in 1.18. I updated the release notes of FLINK-15736 [3] to reflect the beta stage of this feature. Does that sound reasonable?
Matthias [1] https://cwiki.apache.org/confluence/display/FLINK/FLIP-317%3A+Upgrade+Kryo+from+2.24.0+to+5.5.0 [2] https://flink.apache.org/roadmap/ [3] https://issues.apache.org/jira/browse/FLINK-15736 On Wed, Sep 6, 2023 at 11:26 PM Jing Ge <j...@ververica.com> wrote: > Hi folks, > > Sorry to come back to this thread, since I saw FLINK-32327 is already > closed and Chesnay announced on 16th June that Flink master branch (at that > time, now it should be both 1.18 and master branches) builds and runs with > Java 17 out-of-the-box. But according to the feedback from Kurt, I'd like > to double confirm with the following question: > > @Chesnay @Mathias: Will the upcoming Flink 1.18.0 release support Java 17 > even if there are known issues with Java records because of kryo 2.x? > Thanks! > > Best regards, > Jing > > > On Sun, Jun 18, 2023 at 8:45 PM Kurt Ostfeld <kurtostf...@proton.me> > wrote: > >> I think there is some confusion: >> >> Chesnay, not me, recently checked in changes into master so that Flink >> will build + test + run with experimental support for Java 17 but with Kryo >> 2.x as-is so this will error with Java records. Chesnay created this >> particular email thread related to this work. >> >> I (Kurt), created a PR+FLIP several weeks ago for upgrading Kryo from 2.x >> to 5.x, with full backward compatibility for existing >> savepoints/checkpoints, that enables Flink to run on Java 17 with support >> for Java records. This isn't merged into master. I haven't gotten much >> feedback on this. >> >> I recently rebased the Kryo upgrade PR onto the master branch, whicch >> includes Chesnay commits. The PR branch was already running successfully on >> Java 17, Chesnay's changes enable Flink to build and run the CI test suite >> in Java 17 as well. However, without the Kryo upgrade, Flink isn't >> compatible with Java records. >> >> I'd be happy to follow the standard process and do the the FLIP vote, but >> before this is ready for a vote, this PR needs review + testing by someone >> other than me. Specifically, I'd like someone to try to create a Flink >> application that tries to break the upgrade process: either confirm that >> everything works or demonstrate an error scenario. >> >> The Kryo PR code is passing all automated CI tests, which include several >> tests covering backwards compatibility scenarios. I also created this >> simple application https://github.com/kurtostfeld/flink-kryo-upgrade-demo >> to create state with Flink 1.17 and test the upgrade process. From what I >> can see it works, but this would definitely need more testing from people >> other than just me. >> >> >> >> ------- Original Message ------- >> On Sunday, June 18th, 2023 at 7:41 AM, Jing Ge <j...@ververica.com.INVALID> >> wrote: >> >> >> > >> > >> > Hi Kurt, >> > >> > Thanks for your contribution. I am a little bit confused about the email >> > title, since your PR[1] is not merged into the master yet. I guess, with >> > "Experimental Java 17 support", you meant it is available on your branch >> > which is based on the master. >> > >> > If I am not mistaken, there is no vote thread of FLIP 317 on ML. Would >> you >> > like to follow the standard process[2] defined by the Flink community? >> > Thanks! >> > >> > >> > Best regards, >> > Jing >> > >> > [1] https://github.com/apache/flink/pull/22660 >> > [2] >> > >> https://cwiki.apache.org/confluence/display/FLINK/Flink+Improvement+Proposals >> > >> > On Sun, Jun 18, 2023 at 1:18 AM Kurt Ostfeld >> kurtostf...@proton.me.invalid >> > >> > wrote: >> > >> > > I built the Flink master branch and tried running this simple Flink >> app >> > > that uses a Java record: >> > > >> > > >> https://github.com/kurtostfeld/flink-kryo-upgrade-demo/blob/main/flink-record-demo/src/main/java/demo/app/Main.java >> > > >> > > It fails with the normal exception that Kryo 2.x throws when you try >> to >> > > serialize a Java record. The full stack trace is here: >> > > https://pastebin.com/HGhGKUWt >> > > >> > > I tried removing this line: >> > > >> > > >> https://github.com/kurtostfeld/flink-kryo-upgrade-demo/blob/main/flink-record-demo/src/main/java/demo/app/Main.java#L36 >> > > >> > > and that had no impact, I got the same error. >> > > >> > > In the other thread, you said that the plan was to use PojoSerializer >> to >> > > serialize records rather than Kryo. Currently, the Flink code bases >> uses >> > > Kryo 2.x by default for generic user data types, and that will fail >> when >> > > the data type is a record or contains records. Ultimately, if Flink >> wants >> > > to fully support Java records, it seems that it has to move off of >> Kryo >> > > 2.x. PojoSerializer is part of what is basically a custom >> serialization >> > > library internal to Flink that is an alternative to Kryo. That's one >> > > option: move off of Kryo to a Flink-internal serialization library. >> The >> > > other two options are upgrade to the new Kryo or use a different >> > > serialization library. >> > > >> > > The Kryo 5.5.0 upgrade PR I submitted ( >> > > https://github.com/apache/flink/pull/22660) with FLIP 317 ( >> > > >> https://cwiki.apache.org/confluence/display/FLINK/FLIP-317%3A+Upgrade+Kryo+from+2.24.0+to+5.5.0 >> ) >> > > works with records. The Flink app linked above that uses records >> works with >> > > the PR and that's what I posted to this mailing list a few weeks ago. >> I >> > > rebased the pull request on to the latest master branch and it's >> passing >> > > all tests. From my testing, it supports stateful upgrades, including >> > > checkpoints. If you can demonstrate a scenario where stateful upgrades >> > > error I can try to resolve that. >> >
