Re: [DISCUSS] Java Record support
Hi, I have opened a draft PR [1] that shows the minimal required changes and a suggested unit test setup for Java version specific tests. There is still some work to be done (run all benchmarks, add more tests for compatibility/migration) If you have time please review / comment on the approach here or on Github. Cheers, Gyula [1] https://github.com/apache/flink/pull/23490 On Wed, Oct 4, 2023 at 7:09 PM Peter Huang wrote: > +1 for the convenience of users. > > On Wed, Oct 4, 2023 at 8:05 AM Matthias Pohl .invalid> > wrote: > > > +1 Sounds like a good idea. > > > > On Wed, Oct 4, 2023 at 5:04 PM Gyula Fóra wrote: > > > > > I will share my initial implementation soon, it seems to be pretty > > > straightforward. > > > > > > Biggest challenge so far is setting tests so we can still compile > against > > > older versions but have tests for records . But I have working proposal > > for > > > that as well. > > > > > > Gyula > > > > > > On Wed, 4 Oct 2023 at 16:45, Chesnay Schepler > > wrote: > > > > > > > Kryo isn't required for this; newer versions do support records but > we > > > > want something like a PojoSerializer for records to be performant. > > > > > > > > The core challenges are > > > > a) detecting records during type extraction > > > > b) ensuring parameters are passed to the constructor in the right > > order. > > > > > > > > From what I remember from my own experiments this shouldn't exactly > > > > /difficult/, but just a bit tedious to integrate into the Type > > > > extraction stack. > > > > > > > > On 04/10/2023 16:14, Őrhidi Mátyás wrote: > > > > > +1 This would be great > > > > > > > > > > On Wed, Oct 4, 2023 at 7:04 AM Gyula Fóra > > > wrote: > > > > > > > > > > Hi All! > > > > > > > > > > Flink 1.18 contains experimental Java 17 support but it misses > > out > > > > > on Java > > > > > records which can be one of the nice benefits of actually using > > > > > newer java > > > > > versions. > > > > > > > > > > There is already a Jira to track this feature [1] but I am not > > > > > aware of any > > > > > previous efforts so far. > > > > > > > > > > Since records have pretty strong guarantees and many users > would > > > > > probably > > > > > want to migrate from their POJOs, I think we should enhance the > > > > > current > > > > > Pojo TypeInfo/Serializer to accommodate for the records. > > > > > > > > > > I experimented with this locally and the changes are not huge > as > > > > > we only > > > > > need to allow instantiating records through the constructor > > instead > > > > of > > > > > setters. This would mean that the serialization format is > > basically > > > > > equivalent to the same non-record pojo, giving us backward > > > > > compatibility > > > > > and all the features of the Pojo serializer for basically free. > > > > > > > > > > We should make sure to not introduce any performance regression > > in > > > > the > > > > > PojoSerializer but I am happy to open a preview PR if there is > > > > > interest. > > > > > > > > > > There were mentions of upgrading Kryo to support this but I > think > > > > that > > > > > would add unnecessary complexity. > > > > > > > > > > What do you all think? > > > > > > > > > > Cheers, > > > > > Gyula > > > > > > > > > > [1] https://issues.apache.org/jira/browse/FLINK-32380 > > > > > > > > > > > > > > >
Re: [DISCUSS] Java Record support
+1 for the convenience of users. On Wed, Oct 4, 2023 at 8:05 AM Matthias Pohl wrote: > +1 Sounds like a good idea. > > On Wed, Oct 4, 2023 at 5:04 PM Gyula Fóra wrote: > > > I will share my initial implementation soon, it seems to be pretty > > straightforward. > > > > Biggest challenge so far is setting tests so we can still compile against > > older versions but have tests for records . But I have working proposal > for > > that as well. > > > > Gyula > > > > On Wed, 4 Oct 2023 at 16:45, Chesnay Schepler > wrote: > > > > > Kryo isn't required for this; newer versions do support records but we > > > want something like a PojoSerializer for records to be performant. > > > > > > The core challenges are > > > a) detecting records during type extraction > > > b) ensuring parameters are passed to the constructor in the right > order. > > > > > > From what I remember from my own experiments this shouldn't exactly > > > /difficult/, but just a bit tedious to integrate into the Type > > > extraction stack. > > > > > > On 04/10/2023 16:14, Őrhidi Mátyás wrote: > > > > +1 This would be great > > > > > > > > On Wed, Oct 4, 2023 at 7:04 AM Gyula Fóra > > wrote: > > > > > > > > Hi All! > > > > > > > > Flink 1.18 contains experimental Java 17 support but it misses > out > > > > on Java > > > > records which can be one of the nice benefits of actually using > > > > newer java > > > > versions. > > > > > > > > There is already a Jira to track this feature [1] but I am not > > > > aware of any > > > > previous efforts so far. > > > > > > > > Since records have pretty strong guarantees and many users would > > > > probably > > > > want to migrate from their POJOs, I think we should enhance the > > > > current > > > > Pojo TypeInfo/Serializer to accommodate for the records. > > > > > > > > I experimented with this locally and the changes are not huge as > > > > we only > > > > need to allow instantiating records through the constructor > instead > > > of > > > > setters. This would mean that the serialization format is > basically > > > > equivalent to the same non-record pojo, giving us backward > > > > compatibility > > > > and all the features of the Pojo serializer for basically free. > > > > > > > > We should make sure to not introduce any performance regression > in > > > the > > > > PojoSerializer but I am happy to open a preview PR if there is > > > > interest. > > > > > > > > There were mentions of upgrading Kryo to support this but I think > > > that > > > > would add unnecessary complexity. > > > > > > > > What do you all think? > > > > > > > > Cheers, > > > > Gyula > > > > > > > > [1] https://issues.apache.org/jira/browse/FLINK-32380 > > > > > > > > > >
Re: [DISCUSS] Java Record support
+1 Sounds like a good idea. On Wed, Oct 4, 2023 at 5:04 PM Gyula Fóra wrote: > I will share my initial implementation soon, it seems to be pretty > straightforward. > > Biggest challenge so far is setting tests so we can still compile against > older versions but have tests for records . But I have working proposal for > that as well. > > Gyula > > On Wed, 4 Oct 2023 at 16:45, Chesnay Schepler wrote: > > > Kryo isn't required for this; newer versions do support records but we > > want something like a PojoSerializer for records to be performant. > > > > The core challenges are > > a) detecting records during type extraction > > b) ensuring parameters are passed to the constructor in the right order. > > > > From what I remember from my own experiments this shouldn't exactly > > /difficult/, but just a bit tedious to integrate into the Type > > extraction stack. > > > > On 04/10/2023 16:14, Őrhidi Mátyás wrote: > > > +1 This would be great > > > > > > On Wed, Oct 4, 2023 at 7:04 AM Gyula Fóra > wrote: > > > > > > Hi All! > > > > > > Flink 1.18 contains experimental Java 17 support but it misses out > > > on Java > > > records which can be one of the nice benefits of actually using > > > newer java > > > versions. > > > > > > There is already a Jira to track this feature [1] but I am not > > > aware of any > > > previous efforts so far. > > > > > > Since records have pretty strong guarantees and many users would > > > probably > > > want to migrate from their POJOs, I think we should enhance the > > > current > > > Pojo TypeInfo/Serializer to accommodate for the records. > > > > > > I experimented with this locally and the changes are not huge as > > > we only > > > need to allow instantiating records through the constructor instead > > of > > > setters. This would mean that the serialization format is basically > > > equivalent to the same non-record pojo, giving us backward > > > compatibility > > > and all the features of the Pojo serializer for basically free. > > > > > > We should make sure to not introduce any performance regression in > > the > > > PojoSerializer but I am happy to open a preview PR if there is > > > interest. > > > > > > There were mentions of upgrading Kryo to support this but I think > > that > > > would add unnecessary complexity. > > > > > > What do you all think? > > > > > > Cheers, > > > Gyula > > > > > > [1] https://issues.apache.org/jira/browse/FLINK-32380 > > > > > >
Re: [DISCUSS] Java Record support
I will share my initial implementation soon, it seems to be pretty straightforward. Biggest challenge so far is setting tests so we can still compile against older versions but have tests for records . But I have working proposal for that as well. Gyula On Wed, 4 Oct 2023 at 16:45, Chesnay Schepler wrote: > Kryo isn't required for this; newer versions do support records but we > want something like a PojoSerializer for records to be performant. > > The core challenges are > a) detecting records during type extraction > b) ensuring parameters are passed to the constructor in the right order. > > From what I remember from my own experiments this shouldn't exactly > /difficult/, but just a bit tedious to integrate into the Type > extraction stack. > > On 04/10/2023 16:14, Őrhidi Mátyás wrote: > > +1 This would be great > > > > On Wed, Oct 4, 2023 at 7:04 AM Gyula Fóra wrote: > > > > Hi All! > > > > Flink 1.18 contains experimental Java 17 support but it misses out > > on Java > > records which can be one of the nice benefits of actually using > > newer java > > versions. > > > > There is already a Jira to track this feature [1] but I am not > > aware of any > > previous efforts so far. > > > > Since records have pretty strong guarantees and many users would > > probably > > want to migrate from their POJOs, I think we should enhance the > > current > > Pojo TypeInfo/Serializer to accommodate for the records. > > > > I experimented with this locally and the changes are not huge as > > we only > > need to allow instantiating records through the constructor instead > of > > setters. This would mean that the serialization format is basically > > equivalent to the same non-record pojo, giving us backward > > compatibility > > and all the features of the Pojo serializer for basically free. > > > > We should make sure to not introduce any performance regression in > the > > PojoSerializer but I am happy to open a preview PR if there is > > interest. > > > > There were mentions of upgrading Kryo to support this but I think > that > > would add unnecessary complexity. > > > > What do you all think? > > > > Cheers, > > Gyula > > > > [1] https://issues.apache.org/jira/browse/FLINK-32380 > > >
Re: [DISCUSS] Java Record support
Kryo isn't required for this; newer versions do support records but we want something like a PojoSerializer for records to be performant. The core challenges are a) detecting records during type extraction b) ensuring parameters are passed to the constructor in the right order. From what I remember from my own experiments this shouldn't exactly /difficult/, but just a bit tedious to integrate into the Type extraction stack. On 04/10/2023 16:14, Őrhidi Mátyás wrote: +1 This would be great On Wed, Oct 4, 2023 at 7:04 AM Gyula Fóra wrote: Hi All! Flink 1.18 contains experimental Java 17 support but it misses out on Java records which can be one of the nice benefits of actually using newer java versions. There is already a Jira to track this feature [1] but I am not aware of any previous efforts so far. Since records have pretty strong guarantees and many users would probably want to migrate from their POJOs, I think we should enhance the current Pojo TypeInfo/Serializer to accommodate for the records. I experimented with this locally and the changes are not huge as we only need to allow instantiating records through the constructor instead of setters. This would mean that the serialization format is basically equivalent to the same non-record pojo, giving us backward compatibility and all the features of the Pojo serializer for basically free. We should make sure to not introduce any performance regression in the PojoSerializer but I am happy to open a preview PR if there is interest. There were mentions of upgrading Kryo to support this but I think that would add unnecessary complexity. What do you all think? Cheers, Gyula [1] https://issues.apache.org/jira/browse/FLINK-32380
Re: [DISCUSS] Java Record support
+1 This would be great On Wed, Oct 4, 2023 at 7:04 AM Gyula Fóra wrote: > Hi All! > > Flink 1.18 contains experimental Java 17 support but it misses out on Java > records which can be one of the nice benefits of actually using newer java > versions. > > There is already a Jira to track this feature [1] but I am not aware of any > previous efforts so far. > > Since records have pretty strong guarantees and many users would probably > want to migrate from their POJOs, I think we should enhance the current > Pojo TypeInfo/Serializer to accommodate for the records. > > I experimented with this locally and the changes are not huge as we only > need to allow instantiating records through the constructor instead of > setters. This would mean that the serialization format is basically > equivalent to the same non-record pojo, giving us backward compatibility > and all the features of the Pojo serializer for basically free. > > We should make sure to not introduce any performance regression in the > PojoSerializer but I am happy to open a preview PR if there is interest. > > There were mentions of upgrading Kryo to support this but I think that > would add unnecessary complexity. > > What do you all think? > > Cheers, > Gyula > > [1] https://issues.apache.org/jira/browse/FLINK-32380 >
