Re: [DISCUSS] Resolve diamond inheritance of Sink.createWriter
The PR is merged, so everything should be back to normal. Thanks everyone involved! Also based on the discussion here, I have updated the FLIP-372 [1]. I tried to incorporate every comment, but in some cases they were contradictory, so please share your opinion on the corresponding thread [2]. Thanks again, Peter [1] - https://cwiki.apache.org/confluence/display/FLINK/FLIP-372%3A+Allow+TwoPhaseCommittingSink+WithPreCommitTopology+to+alter+the+type+of+the+Committable [2] - https://lists.apache.org/thread/344pzbrqbbb4w0sfj67km25msp7hxlyd Leonard Xu ezt írta (időpont: 2023. dec. 12., K, 7:33): > Thanks Peter and Marton for the quick response and fix, I’ve left +1 for > this PR. > > I’d like to take a look at followup FLIP-372 as well. > > Best, > Leonard > > > > 2023年12月12日 上午7:39，Becket Qin 写道： > > > > Hi Peter, > > > > Thanks for updating the patch. The latest patch looks good to me. I've > +1ed > > on the PR. > > > > Cheers, > > > > Jiangjie (Becket) Qin > > > > On Mon, Dec 11, 2023 at 9:21 PM Péter Váry > > wrote: > > > >> Thanks everyone for the lively discussion! > >> > >> The PR is available which strictly adheres the accepted changes from > >> FLIP-371. Thanks Gyula and Marton for the review. Becket, if you have > any > >> questions left, please let me know, so I can fix and we can merge the > >> changes. > >> > >> I would like to invite everyone involved here to take a look at FLIP-372 > >> [1], and the related mailing thread [2]. The discussion there is also at > >> the stage where we are debating the merits of migrating to a mixin based > >> Sink API. So if you are interested, please join us there. > >> > >> Thanks, > >> Peter > >> > >> [1] - > >> > >> > https://cwiki.apache.org/confluence/display/FLINK/FLIP-372%3A+Allow+TwoPhaseCommittingSink+WithPreCommitTopology+to+alter+the+type+of+the+Committable > >> [2] - https://lists.apache.org/thread/344pzbrqbbb4w0sfj67km25msp7hxlyd > >> > >> > >> On Tue, Dec 5, 2023, 18:05 Márton Balassi > >> wrote: > >> > >>> Thanks, Peter. Given the discussion I also agree that the consensus is > to > >>> move towards the mixin interface approach (and accept its disadvantages > >>> given its advantages). > >>> > >>> +1 for the general direction of your proposed code change in > >>> https://github.com/apache/flink/pull/23876. > >>> > >>> On Tue, Dec 5, 2023 at 3:44 PM Péter Váry > > >>> wrote: > >>> > It seems to me we have a consensus to move forward with the mixin > >>> approach. > I hope that everyone is aware that with the mixin interfaces we lose > >> the > opportunity of the strong type checks. This will be especially painful > >>> for > generic types, where we will not have a way to ensure that the generic > types are correctly synchronized between the different interfaces, > even > >>> on > DAG creation time. > > Even with this drawback, I like this approach too, so +1 from my side. > > As a first step in the direction of the mixin approach, we can remove > >> the > specific implementations of the `createWriter` methods from the > `StatefulSink` and the `TwoPhaseCommitingSink` interfaces (and replace > >>> them > with an instanceof check where needed). > - This would remove the diamond inheritance - enable us to create > >> default > methods for backward compatibility. > - This would not break the API, as the same method with wider return > >>> value > will be inherited from the `Sink` interface. > > Since, it might be easier to understand the proposed changes, I have > created a new PR: https://github.com/apache/flink/pull/23876 > The PR has 2 commits: > - Reverting the previous change - non-clean, since there were some > additional fixes on the tests - > > > >>> > >> > https://github.com/apache/flink/pull/23876/commits/c7625d5fa62a6e9a182f39f53fb7e5626105f3b0 > - The new change with mixin approach, and deprecation - > > > >>> > >> > https://github.com/apache/flink/pull/23876/commits/99ec936966af527598ca49712c1263bc4aa03c15 > > Thanks, > Peter > > weijie guo ezt írta (időpont: 2023. dec. > >> 5., > K, > 8:01): > > > Thanks Martijn for driving this! > > > > I'm +1 to reverting the breaking change. > > > >> For new functionality or changes we can make easily, we should > >> switch > to > > the decorative/mixin interface approach used successfully in the > >> source > and > > table interfaces. > > > > I like the way of switching to mixin interface. > > > > Best regards, > > > > Weijie > > > > > > Becket Qin 于2023年12月5日周二 14:50写道： > > > >> I am with Gyula about fixing the current SinkV2 API. > >> > >> A SinkV3 seems not necessary because we are not changing the > fundamental > >> design of the API. Hopefully we can modify the interface structure > >> a > > little > >>
Re: [DISCUSS] Resolve diamond inheritance of Sink.createWriter
Thanks Peter and Marton for the quick response and fix, I’ve left +1 for this PR. I’d like to take a look at followup FLIP-372 as well. Best, Leonard > 2023年12月12日 上午7:39，Becket Qin 写道： > > Hi Peter, > > Thanks for updating the patch. The latest patch looks good to me. I've +1ed > on the PR. > > Cheers, > > Jiangjie (Becket) Qin > > On Mon, Dec 11, 2023 at 9:21 PM Péter Váry > wrote: > >> Thanks everyone for the lively discussion! >> >> The PR is available which strictly adheres the accepted changes from >> FLIP-371. Thanks Gyula and Marton for the review. Becket, if you have any >> questions left, please let me know, so I can fix and we can merge the >> changes. >> >> I would like to invite everyone involved here to take a look at FLIP-372 >> [1], and the related mailing thread [2]. The discussion there is also at >> the stage where we are debating the merits of migrating to a mixin based >> Sink API. So if you are interested, please join us there. >> >> Thanks, >> Peter >> >> [1] - >> >> https://cwiki.apache.org/confluence/display/FLINK/FLIP-372%3A+Allow+TwoPhaseCommittingSink+WithPreCommitTopology+to+alter+the+type+of+the+Committable >> [2] - https://lists.apache.org/thread/344pzbrqbbb4w0sfj67km25msp7hxlyd >> >> >> On Tue, Dec 5, 2023, 18:05 Márton Balassi >> wrote: >> >>> Thanks, Peter. Given the discussion I also agree that the consensus is to >>> move towards the mixin interface approach (and accept its disadvantages >>> given its advantages). >>> >>> +1 for the general direction of your proposed code change in >>> https://github.com/apache/flink/pull/23876. >>> >>> On Tue, Dec 5, 2023 at 3:44 PM Péter Váry >>> wrote: >>> It seems to me we have a consensus to move forward with the mixin >>> approach. I hope that everyone is aware that with the mixin interfaces we lose >> the opportunity of the strong type checks. This will be especially painful >>> for generic types, where we will not have a way to ensure that the generic types are correctly synchronized between the different interfaces, even >>> on DAG creation time. Even with this drawback, I like this approach too, so +1 from my side. As a first step in the direction of the mixin approach, we can remove >> the specific implementations of the `createWriter` methods from the `StatefulSink` and the `TwoPhaseCommitingSink` interfaces (and replace >>> them with an instanceof check where needed). - This would remove the diamond inheritance - enable us to create >> default methods for backward compatibility. - This would not break the API, as the same method with wider return >>> value will be inherited from the `Sink` interface. Since, it might be easier to understand the proposed changes, I have created a new PR: https://github.com/apache/flink/pull/23876 The PR has 2 commits: - Reverting the previous change - non-clean, since there were some additional fixes on the tests - >>> >> https://github.com/apache/flink/pull/23876/commits/c7625d5fa62a6e9a182f39f53fb7e5626105f3b0 - The new change with mixin approach, and deprecation - >>> >> https://github.com/apache/flink/pull/23876/commits/99ec936966af527598ca49712c1263bc4aa03c15 Thanks, Peter weijie guo ezt írta (időpont: 2023. dec. >> 5., K, 8:01): > Thanks Martijn for driving this! > > I'm +1 to reverting the breaking change. > >> For new functionality or changes we can make easily, we should >> switch to > the decorative/mixin interface approach used successfully in the >> source and > table interfaces. > > I like the way of switching to mixin interface. > > Best regards, > > Weijie > > > Becket Qin 于2023年12月5日周二 14:50写道： > >> I am with Gyula about fixing the current SinkV2 API. >> >> A SinkV3 seems not necessary because we are not changing the fundamental >> design of the API. Hopefully we can modify the interface structure >> a > little >> bit to make it similar to the Source while still keep the backwards >> compatibility. >> For example, one approach is: >> >> - Add snapshotState(int checkpointId) and precommit() methods to >> the >> SinkWriter with default implementation doing nothing. Deprecate >> StatefulSinkWriter and PrecommittingSinkWriter. >> - Add two mixin interfaces of SupportsStatefulWrite and >> SupportsTwoPhaseCommit. Deprecate the StatefulSink and >> TwoPhaseCommittingSink. >> >> Thanks, >> >> Jiangjie (Becket) Qin >> >> On Mon, Dec 4, 2023 at 7:25 PM Gyula Fóra wrote: >> >>> Hi All! >>> >>> Based on the discussion above, I feel that the most reasonable approach >>> from both developers and users perspective at this point is what Becket >>> lists as
Re: [DISCUSS] Resolve diamond inheritance of Sink.createWriter
Hi Peter, Thanks for updating the patch. The latest patch looks good to me. I've +1ed on the PR. Cheers, Jiangjie (Becket) Qin On Mon, Dec 11, 2023 at 9:21 PM Péter Váry wrote: > Thanks everyone for the lively discussion! > > The PR is available which strictly adheres the accepted changes from > FLIP-371. Thanks Gyula and Marton for the review. Becket, if you have any > questions left, please let me know, so I can fix and we can merge the > changes. > > I would like to invite everyone involved here to take a look at FLIP-372 > [1], and the related mailing thread [2]. The discussion there is also at > the stage where we are debating the merits of migrating to a mixin based > Sink API. So if you are interested, please join us there. > > Thanks, > Peter > > [1] - > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-372%3A+Allow+TwoPhaseCommittingSink+WithPreCommitTopology+to+alter+the+type+of+the+Committable > [2] - https://lists.apache.org/thread/344pzbrqbbb4w0sfj67km25msp7hxlyd > > > On Tue, Dec 5, 2023, 18:05 Márton Balassi > wrote: > > > Thanks, Peter. Given the discussion I also agree that the consensus is to > > move towards the mixin interface approach (and accept its disadvantages > > given its advantages). > > > > +1 for the general direction of your proposed code change in > > https://github.com/apache/flink/pull/23876. > > > > On Tue, Dec 5, 2023 at 3:44 PM Péter Váry > > wrote: > > > > > It seems to me we have a consensus to move forward with the mixin > > approach. > > > I hope that everyone is aware that with the mixin interfaces we lose > the > > > opportunity of the strong type checks. This will be especially painful > > for > > > generic types, where we will not have a way to ensure that the generic > > > types are correctly synchronized between the different interfaces, even > > on > > > DAG creation time. > > > > > > Even with this drawback, I like this approach too, so +1 from my side. > > > > > > As a first step in the direction of the mixin approach, we can remove > the > > > specific implementations of the `createWriter` methods from the > > > `StatefulSink` and the `TwoPhaseCommitingSink` interfaces (and replace > > them > > > with an instanceof check where needed). > > > - This would remove the diamond inheritance - enable us to create > default > > > methods for backward compatibility. > > > - This would not break the API, as the same method with wider return > > value > > > will be inherited from the `Sink` interface. > > > > > > Since, it might be easier to understand the proposed changes, I have > > > created a new PR: https://github.com/apache/flink/pull/23876 > > > The PR has 2 commits: > > > - Reverting the previous change - non-clean, since there were some > > > additional fixes on the tests - > > > > > > > > > https://github.com/apache/flink/pull/23876/commits/c7625d5fa62a6e9a182f39f53fb7e5626105f3b0 > > > - The new change with mixin approach, and deprecation - > > > > > > > > > https://github.com/apache/flink/pull/23876/commits/99ec936966af527598ca49712c1263bc4aa03c15 > > > > > > Thanks, > > > Peter > > > > > > weijie guo ezt írta (időpont: 2023. dec. > 5., > > > K, > > > 8:01): > > > > > > > Thanks Martijn for driving this! > > > > > > > > I'm +1 to reverting the breaking change. > > > > > > > > > For new functionality or changes we can make easily, we should > switch > > > to > > > > the decorative/mixin interface approach used successfully in the > source > > > and > > > > table interfaces. > > > > > > > > I like the way of switching to mixin interface. > > > > > > > > Best regards, > > > > > > > > Weijie > > > > > > > > > > > > Becket Qin 于2023年12月5日周二 14:50写道： > > > > > > > > > I am with Gyula about fixing the current SinkV2 API. > > > > > > > > > > A SinkV3 seems not necessary because we are not changing the > > > fundamental > > > > > design of the API. Hopefully we can modify the interface structure > a > > > > little > > > > > bit to make it similar to the Source while still keep the backwards > > > > > compatibility. > > > > > For example, one approach is: > > > > > > > > > > - Add snapshotState(int checkpointId) and precommit() methods to > the > > > > > SinkWriter with default implementation doing nothing. Deprecate > > > > > StatefulSinkWriter and PrecommittingSinkWriter. > > > > > - Add two mixin interfaces of SupportsStatefulWrite and > > > > > SupportsTwoPhaseCommit. Deprecate the StatefulSink and > > > > > TwoPhaseCommittingSink. > > > > > > > > > > Thanks, > > > > > > > > > > Jiangjie (Becket) Qin > > > > > > > > > > On Mon, Dec 4, 2023 at 7:25 PM Gyula Fóra > > > wrote: > > > > > > > > > > > Hi All! > > > > > > > > > > > > Based on the discussion above, I feel that the most reasonable > > > approach > > > > > > from both developers and users perspective at this point is what > > > Becket > > > > > > lists as Option 1: > > > > > > > > > > > > Revert the naming change to the backward compatible version and > > > accept > > > > > that > > >
Re: [DISCUSS] Resolve diamond inheritance of Sink.createWriter
Thanks everyone for the lively discussion! The PR is available which strictly adheres the accepted changes from FLIP-371. Thanks Gyula and Marton for the review. Becket, if you have any questions left, please let me know, so I can fix and we can merge the changes. I would like to invite everyone involved here to take a look at FLIP-372 [1], and the related mailing thread [2]. The discussion there is also at the stage where we are debating the merits of migrating to a mixin based Sink API. So if you are interested, please join us there. Thanks, Peter [1] - https://cwiki.apache.org/confluence/display/FLINK/FLIP-372%3A+Allow+TwoPhaseCommittingSink+WithPreCommitTopology+to+alter+the+type+of+the+Committable [2] - https://lists.apache.org/thread/344pzbrqbbb4w0sfj67km25msp7hxlyd On Tue, Dec 5, 2023, 18:05 Márton Balassi wrote: > Thanks, Peter. Given the discussion I also agree that the consensus is to > move towards the mixin interface approach (and accept its disadvantages > given its advantages). > > +1 for the general direction of your proposed code change in > https://github.com/apache/flink/pull/23876. > > On Tue, Dec 5, 2023 at 3:44 PM Péter Váry > wrote: > > > It seems to me we have a consensus to move forward with the mixin > approach. > > I hope that everyone is aware that with the mixin interfaces we lose the > > opportunity of the strong type checks. This will be especially painful > for > > generic types, where we will not have a way to ensure that the generic > > types are correctly synchronized between the different interfaces, even > on > > DAG creation time. > > > > Even with this drawback, I like this approach too, so +1 from my side. > > > > As a first step in the direction of the mixin approach, we can remove the > > specific implementations of the `createWriter` methods from the > > `StatefulSink` and the `TwoPhaseCommitingSink` interfaces (and replace > them > > with an instanceof check where needed). > > - This would remove the diamond inheritance - enable us to create default > > methods for backward compatibility. > > - This would not break the API, as the same method with wider return > value > > will be inherited from the `Sink` interface. > > > > Since, it might be easier to understand the proposed changes, I have > > created a new PR: https://github.com/apache/flink/pull/23876 > > The PR has 2 commits: > > - Reverting the previous change - non-clean, since there were some > > additional fixes on the tests - > > > > > https://github.com/apache/flink/pull/23876/commits/c7625d5fa62a6e9a182f39f53fb7e5626105f3b0 > > - The new change with mixin approach, and deprecation - > > > > > https://github.com/apache/flink/pull/23876/commits/99ec936966af527598ca49712c1263bc4aa03c15 > > > > Thanks, > > Peter > > > > weijie guo ezt írta (időpont: 2023. dec. 5., > > K, > > 8:01): > > > > > Thanks Martijn for driving this! > > > > > > I'm +1 to reverting the breaking change. > > > > > > > For new functionality or changes we can make easily, we should switch > > to > > > the decorative/mixin interface approach used successfully in the source > > and > > > table interfaces. > > > > > > I like the way of switching to mixin interface. > > > > > > Best regards, > > > > > > Weijie > > > > > > > > > Becket Qin 于2023年12月5日周二 14:50写道： > > > > > > > I am with Gyula about fixing the current SinkV2 API. > > > > > > > > A SinkV3 seems not necessary because we are not changing the > > fundamental > > > > design of the API. Hopefully we can modify the interface structure a > > > little > > > > bit to make it similar to the Source while still keep the backwards > > > > compatibility. > > > > For example, one approach is: > > > > > > > > - Add snapshotState(int checkpointId) and precommit() methods to the > > > > SinkWriter with default implementation doing nothing. Deprecate > > > > StatefulSinkWriter and PrecommittingSinkWriter. > > > > - Add two mixin interfaces of SupportsStatefulWrite and > > > > SupportsTwoPhaseCommit. Deprecate the StatefulSink and > > > > TwoPhaseCommittingSink. > > > > > > > > Thanks, > > > > > > > > Jiangjie (Becket) Qin > > > > > > > > On Mon, Dec 4, 2023 at 7:25 PM Gyula Fóra > > wrote: > > > > > > > > > Hi All! > > > > > > > > > > Based on the discussion above, I feel that the most reasonable > > approach > > > > > from both developers and users perspective at this point is what > > Becket > > > > > lists as Option 1: > > > > > > > > > > Revert the naming change to the backward compatible version and > > accept > > > > that > > > > > the names are not perfect (treat it as legacy). > > > > > > > > > > On a different note, I agree that the current sink v2 interface is > > very > > > > > difficult to evolve and structuring the interfaces the way they are > > now > > > > is > > > > > not a good design in the long run. > > > > > For new functionality or changes we can make easily, we should > switch > > > to > > > > > the decorative/mixin interface approach used successfully in the >
Re: [DISCUSS] Resolve diamond inheritance of Sink.createWriter
Thanks, Peter. Given the discussion I also agree that the consensus is to move towards the mixin interface approach (and accept its disadvantages given its advantages). +1 for the general direction of your proposed code change in https://github.com/apache/flink/pull/23876. On Tue, Dec 5, 2023 at 3:44 PM Péter Váry wrote: > It seems to me we have a consensus to move forward with the mixin approach. > I hope that everyone is aware that with the mixin interfaces we lose the > opportunity of the strong type checks. This will be especially painful for > generic types, where we will not have a way to ensure that the generic > types are correctly synchronized between the different interfaces, even on > DAG creation time. > > Even with this drawback, I like this approach too, so +1 from my side. > > As a first step in the direction of the mixin approach, we can remove the > specific implementations of the `createWriter` methods from the > `StatefulSink` and the `TwoPhaseCommitingSink` interfaces (and replace them > with an instanceof check where needed). > - This would remove the diamond inheritance - enable us to create default > methods for backward compatibility. > - This would not break the API, as the same method with wider return value > will be inherited from the `Sink` interface. > > Since, it might be easier to understand the proposed changes, I have > created a new PR: https://github.com/apache/flink/pull/23876 > The PR has 2 commits: > - Reverting the previous change - non-clean, since there were some > additional fixes on the tests - > > https://github.com/apache/flink/pull/23876/commits/c7625d5fa62a6e9a182f39f53fb7e5626105f3b0 > - The new change with mixin approach, and deprecation - > > https://github.com/apache/flink/pull/23876/commits/99ec936966af527598ca49712c1263bc4aa03c15 > > Thanks, > Peter > > weijie guo ezt írta (időpont: 2023. dec. 5., > K, > 8:01): > > > Thanks Martijn for driving this! > > > > I'm +1 to reverting the breaking change. > > > > > For new functionality or changes we can make easily, we should switch > to > > the decorative/mixin interface approach used successfully in the source > and > > table interfaces. > > > > I like the way of switching to mixin interface. > > > > Best regards, > > > > Weijie > > > > > > Becket Qin 于2023年12月5日周二 14:50写道： > > > > > I am with Gyula about fixing the current SinkV2 API. > > > > > > A SinkV3 seems not necessary because we are not changing the > fundamental > > > design of the API. Hopefully we can modify the interface structure a > > little > > > bit to make it similar to the Source while still keep the backwards > > > compatibility. > > > For example, one approach is: > > > > > > - Add snapshotState(int checkpointId) and precommit() methods to the > > > SinkWriter with default implementation doing nothing. Deprecate > > > StatefulSinkWriter and PrecommittingSinkWriter. > > > - Add two mixin interfaces of SupportsStatefulWrite and > > > SupportsTwoPhaseCommit. Deprecate the StatefulSink and > > > TwoPhaseCommittingSink. > > > > > > Thanks, > > > > > > Jiangjie (Becket) Qin > > > > > > On Mon, Dec 4, 2023 at 7:25 PM Gyula Fóra > wrote: > > > > > > > Hi All! > > > > > > > > Based on the discussion above, I feel that the most reasonable > approach > > > > from both developers and users perspective at this point is what > Becket > > > > lists as Option 1: > > > > > > > > Revert the naming change to the backward compatible version and > accept > > > that > > > > the names are not perfect (treat it as legacy). > > > > > > > > On a different note, I agree that the current sink v2 interface is > very > > > > difficult to evolve and structuring the interfaces the way they are > now > > > is > > > > not a good design in the long run. > > > > For new functionality or changes we can make easily, we should switch > > to > > > > the decorative/mixin interface approach used successfully in the > source > > > and > > > > table interfaces. Let's try to do this as much as possible within the > > v2 > > > > and compatibility boundaries and we should only introduce a v3 if we > > > really > > > > must. > > > > > > > > So from my side, +1 to reverting the naming to keep backward > > > compatibility. > > > > > > > > Cheers, > > > > Gyula > > > > > > > > > > > > On Fri, Dec 1, 2023 at 10:43 AM Péter Váry < > > peter.vary.apa...@gmail.com> > > > > wrote: > > > > > > > > > Thanks Becket for your reply! > > > > > > > > > > *On Option 1:* > > > > > - I personally consider API inconsistencies more important, since > > they > > > > will > > > > > remain with us "forever", but this is up to the community. I can > > > > implement > > > > > whichever solution we decide upon. > > > > > > > > > > *Option 2:* > > > > > - I don't think this specific issue merits a rewrite, but if we > > decide > > > to > > > > > change our approach, then it's a different story. > > > > > > > > > > *Evolvability:* > > > > > This discussion reminds me of a similar discussion on FLIP-372
Re: [DISCUSS] Resolve diamond inheritance of Sink.createWriter
It seems to me we have a consensus to move forward with the mixin approach. I hope that everyone is aware that with the mixin interfaces we lose the opportunity of the strong type checks. This will be especially painful for generic types, where we will not have a way to ensure that the generic types are correctly synchronized between the different interfaces, even on DAG creation time. Even with this drawback, I like this approach too, so +1 from my side. As a first step in the direction of the mixin approach, we can remove the specific implementations of the `createWriter` methods from the `StatefulSink` and the `TwoPhaseCommitingSink` interfaces (and replace them with an instanceof check where needed). - This would remove the diamond inheritance - enable us to create default methods for backward compatibility. - This would not break the API, as the same method with wider return value will be inherited from the `Sink` interface. Since, it might be easier to understand the proposed changes, I have created a new PR: https://github.com/apache/flink/pull/23876 The PR has 2 commits: - Reverting the previous change - non-clean, since there were some additional fixes on the tests - https://github.com/apache/flink/pull/23876/commits/c7625d5fa62a6e9a182f39f53fb7e5626105f3b0 - The new change with mixin approach, and deprecation - https://github.com/apache/flink/pull/23876/commits/99ec936966af527598ca49712c1263bc4aa03c15 Thanks, Peter weijie guo ezt írta (időpont: 2023. dec. 5., K, 8:01): > Thanks Martijn for driving this! > > I'm +1 to reverting the breaking change. > > > For new functionality or changes we can make easily, we should switch to > the decorative/mixin interface approach used successfully in the source and > table interfaces. > > I like the way of switching to mixin interface. > > Best regards, > > Weijie > > > Becket Qin 于2023年12月5日周二 14:50写道： > > > I am with Gyula about fixing the current SinkV2 API. > > > > A SinkV3 seems not necessary because we are not changing the fundamental > > design of the API. Hopefully we can modify the interface structure a > little > > bit to make it similar to the Source while still keep the backwards > > compatibility. > > For example, one approach is: > > > > - Add snapshotState(int checkpointId) and precommit() methods to the > > SinkWriter with default implementation doing nothing. Deprecate > > StatefulSinkWriter and PrecommittingSinkWriter. > > - Add two mixin interfaces of SupportsStatefulWrite and > > SupportsTwoPhaseCommit. Deprecate the StatefulSink and > > TwoPhaseCommittingSink. > > > > Thanks, > > > > Jiangjie (Becket) Qin > > > > On Mon, Dec 4, 2023 at 7:25 PM Gyula Fóra wrote: > > > > > Hi All! > > > > > > Based on the discussion above, I feel that the most reasonable approach > > > from both developers and users perspective at this point is what Becket > > > lists as Option 1: > > > > > > Revert the naming change to the backward compatible version and accept > > that > > > the names are not perfect (treat it as legacy). > > > > > > On a different note, I agree that the current sink v2 interface is very > > > difficult to evolve and structuring the interfaces the way they are now > > is > > > not a good design in the long run. > > > For new functionality or changes we can make easily, we should switch > to > > > the decorative/mixin interface approach used successfully in the source > > and > > > table interfaces. Let's try to do this as much as possible within the > v2 > > > and compatibility boundaries and we should only introduce a v3 if we > > really > > > must. > > > > > > So from my side, +1 to reverting the naming to keep backward > > compatibility. > > > > > > Cheers, > > > Gyula > > > > > > > > > On Fri, Dec 1, 2023 at 10:43 AM Péter Váry < > peter.vary.apa...@gmail.com> > > > wrote: > > > > > > > Thanks Becket for your reply! > > > > > > > > *On Option 1:* > > > > - I personally consider API inconsistencies more important, since > they > > > will > > > > remain with us "forever", but this is up to the community. I can > > > implement > > > > whichever solution we decide upon. > > > > > > > > *Option 2:* > > > > - I don't think this specific issue merits a rewrite, but if we > decide > > to > > > > change our approach, then it's a different story. > > > > > > > > *Evolvability:* > > > > This discussion reminds me of a similar discussion on FLIP-372 [1], > > where > > > > we are trying to decide if we should use mixin interfaces, or use > > > interface > > > > inheritance. > > > > With the mixin approach, we have a more flexible interface, but we > > can't > > > > check the generic types of the interfaces/classes on compile time, or > > > even > > > > when we create the DAG. The first issue happens when we call the > method > > > and > > > > fail. > > > > The issue here is similar: > > > > - *StatefulSink* needs a writer with a method to `*snapshotState*` > > > > - *TwoPhaseCommittingSink* needs a writer with `*prepareCommit*` > > > > -
Re: [DISCUSS] Resolve diamond inheritance of Sink.createWriter
Thanks Martijn for driving this! I'm +1 to reverting the breaking change. > For new functionality or changes we can make easily, we should switch to the decorative/mixin interface approach used successfully in the source and table interfaces. I like the way of switching to mixin interface. Best regards, Weijie Becket Qin 于2023年12月5日周二 14:50写道： > I am with Gyula about fixing the current SinkV2 API. > > A SinkV3 seems not necessary because we are not changing the fundamental > design of the API. Hopefully we can modify the interface structure a little > bit to make it similar to the Source while still keep the backwards > compatibility. > For example, one approach is: > > - Add snapshotState(int checkpointId) and precommit() methods to the > SinkWriter with default implementation doing nothing. Deprecate > StatefulSinkWriter and PrecommittingSinkWriter. > - Add two mixin interfaces of SupportsStatefulWrite and > SupportsTwoPhaseCommit. Deprecate the StatefulSink and > TwoPhaseCommittingSink. > > Thanks, > > Jiangjie (Becket) Qin > > On Mon, Dec 4, 2023 at 7:25 PM Gyula Fóra wrote: > > > Hi All! > > > > Based on the discussion above, I feel that the most reasonable approach > > from both developers and users perspective at this point is what Becket > > lists as Option 1: > > > > Revert the naming change to the backward compatible version and accept > that > > the names are not perfect (treat it as legacy). > > > > On a different note, I agree that the current sink v2 interface is very > > difficult to evolve and structuring the interfaces the way they are now > is > > not a good design in the long run. > > For new functionality or changes we can make easily, we should switch to > > the decorative/mixin interface approach used successfully in the source > and > > table interfaces. Let's try to do this as much as possible within the v2 > > and compatibility boundaries and we should only introduce a v3 if we > really > > must. > > > > So from my side, +1 to reverting the naming to keep backward > compatibility. > > > > Cheers, > > Gyula > > > > > > On Fri, Dec 1, 2023 at 10:43 AM Péter Váry > > wrote: > > > > > Thanks Becket for your reply! > > > > > > *On Option 1:* > > > - I personally consider API inconsistencies more important, since they > > will > > > remain with us "forever", but this is up to the community. I can > > implement > > > whichever solution we decide upon. > > > > > > *Option 2:* > > > - I don't think this specific issue merits a rewrite, but if we decide > to > > > change our approach, then it's a different story. > > > > > > *Evolvability:* > > > This discussion reminds me of a similar discussion on FLIP-372 [1], > where > > > we are trying to decide if we should use mixin interfaces, or use > > interface > > > inheritance. > > > With the mixin approach, we have a more flexible interface, but we > can't > > > check the generic types of the interfaces/classes on compile time, or > > even > > > when we create the DAG. The first issue happens when we call the method > > and > > > fail. > > > The issue here is similar: > > > - *StatefulSink* needs a writer with a method to `*snapshotState*` > > > - *TwoPhaseCommittingSink* needs a writer with `*prepareCommit*` > > > - If there is a Sink which is stateful and needs to commit, then it > needs > > > both of these methods. > > > > > > If we use the mixin solution here, we lose the possibility to check the > > > types in compile time. We could do the type check in runtime using ` > > > *instanceof*`, so we are better off than with the FLIP-372 example > above, > > > but still lose any important possibility. I personally prefer the mixin > > > approach, but that would mean we rewrite the Sink API again - likely a > > > SinkV3. Are we ready to move down that path? > > > > > > Thanks, > > > Peter > > > > > > [1] - https://lists.apache.org/thread/344pzbrqbbb4w0sfj67km25msp7hxlyd > > > > > > On Thu, Nov 30, 2023, 14:53 Becket Qin wrote: > > > > > > > Hi folks, > > > > > > > > Sorry for replying late on the thread. > > > > > > > > For this particular FLIP, I see two solutions: > > > > > > > > Option 1: > > > > 1. On top of the the current status, rename > > > > *org.apache.flink.api.connector.sink2.InitContext *to > > > > *CommonInitContext (*should > > > > probably be package private*)*. > > > > 2. Change the name *WriterInitContext* back to *InitContext, *and > > revert > > > > the deprecation. We can change the parameter name to writerContext if > > we > > > > want to. > > > > Admittedly, this does not have full symmetric naming of the > > InitContexts > > > - > > > > we will have CommonInitContext / InitContext / CommitterInitContext > > > instead > > > > of InitContext / WriterInitContext / CommitterInitContext. However, > the > > > > naming seems clear without much confusion. Personally, I can live > with > > > > that, treating the class InitContext as a non-ideal legacy class name > > > > without much material harm. > > > > > > > >
Re: [DISCUSS] Resolve diamond inheritance of Sink.createWriter
I am with Gyula about fixing the current SinkV2 API. A SinkV3 seems not necessary because we are not changing the fundamental design of the API. Hopefully we can modify the interface structure a little bit to make it similar to the Source while still keep the backwards compatibility. For example, one approach is: - Add snapshotState(int checkpointId) and precommit() methods to the SinkWriter with default implementation doing nothing. Deprecate StatefulSinkWriter and PrecommittingSinkWriter. - Add two mixin interfaces of SupportsStatefulWrite and SupportsTwoPhaseCommit. Deprecate the StatefulSink and TwoPhaseCommittingSink. Thanks, Jiangjie (Becket) Qin On Mon, Dec 4, 2023 at 7:25 PM Gyula Fóra wrote: > Hi All! > > Based on the discussion above, I feel that the most reasonable approach > from both developers and users perspective at this point is what Becket > lists as Option 1: > > Revert the naming change to the backward compatible version and accept that > the names are not perfect (treat it as legacy). > > On a different note, I agree that the current sink v2 interface is very > difficult to evolve and structuring the interfaces the way they are now is > not a good design in the long run. > For new functionality or changes we can make easily, we should switch to > the decorative/mixin interface approach used successfully in the source and > table interfaces. Let's try to do this as much as possible within the v2 > and compatibility boundaries and we should only introduce a v3 if we really > must. > > So from my side, +1 to reverting the naming to keep backward compatibility. > > Cheers, > Gyula > > > On Fri, Dec 1, 2023 at 10:43 AM Péter Váry > wrote: > > > Thanks Becket for your reply! > > > > *On Option 1:* > > - I personally consider API inconsistencies more important, since they > will > > remain with us "forever", but this is up to the community. I can > implement > > whichever solution we decide upon. > > > > *Option 2:* > > - I don't think this specific issue merits a rewrite, but if we decide to > > change our approach, then it's a different story. > > > > *Evolvability:* > > This discussion reminds me of a similar discussion on FLIP-372 [1], where > > we are trying to decide if we should use mixin interfaces, or use > interface > > inheritance. > > With the mixin approach, we have a more flexible interface, but we can't > > check the generic types of the interfaces/classes on compile time, or > even > > when we create the DAG. The first issue happens when we call the method > and > > fail. > > The issue here is similar: > > - *StatefulSink* needs a writer with a method to `*snapshotState*` > > - *TwoPhaseCommittingSink* needs a writer with `*prepareCommit*` > > - If there is a Sink which is stateful and needs to commit, then it needs > > both of these methods. > > > > If we use the mixin solution here, we lose the possibility to check the > > types in compile time. We could do the type check in runtime using ` > > *instanceof*`, so we are better off than with the FLIP-372 example above, > > but still lose any important possibility. I personally prefer the mixin > > approach, but that would mean we rewrite the Sink API again - likely a > > SinkV3. Are we ready to move down that path? > > > > Thanks, > > Peter > > > > [1] - https://lists.apache.org/thread/344pzbrqbbb4w0sfj67km25msp7hxlyd > > > > On Thu, Nov 30, 2023, 14:53 Becket Qin wrote: > > > > > Hi folks, > > > > > > Sorry for replying late on the thread. > > > > > > For this particular FLIP, I see two solutions: > > > > > > Option 1: > > > 1. On top of the the current status, rename > > > *org.apache.flink.api.connector.sink2.InitContext *to > > > *CommonInitContext (*should > > > probably be package private*)*. > > > 2. Change the name *WriterInitContext* back to *InitContext, *and > revert > > > the deprecation. We can change the parameter name to writerContext if > we > > > want to. > > > Admittedly, this does not have full symmetric naming of the > InitContexts > > - > > > we will have CommonInitContext / InitContext / CommitterInitContext > > instead > > > of InitContext / WriterInitContext / CommitterInitContext. However, the > > > naming seems clear without much confusion. Personally, I can live with > > > that, treating the class InitContext as a non-ideal legacy class name > > > without much material harm. > > > > > > Option 2: > > > Theoretically speaking, if we really want to reach the perfect state > > while > > > being backwards compatible, we can create a brand new set of Sink > > > interfaces and deprecate the old ones. But I feel this is an overkill > > here. > > > > > > The solution to this particular issue aside, the evolvability of the > > > current interface hierarchy seems a more fundamental issue and worries > me > > > more. I haven't completely thought it through, but there are two > > noticeable > > > differences between the interface design principles between Source and > > > Sink. > > > 1. Source uses
Re: [DISCUSS] Resolve diamond inheritance of Sink.createWriter
Hi All! Based on the discussion above, I feel that the most reasonable approach from both developers and users perspective at this point is what Becket lists as Option 1: Revert the naming change to the backward compatible version and accept that the names are not perfect (treat it as legacy). On a different note, I agree that the current sink v2 interface is very difficult to evolve and structuring the interfaces the way they are now is not a good design in the long run. For new functionality or changes we can make easily, we should switch to the decorative/mixin interface approach used successfully in the source and table interfaces. Let's try to do this as much as possible within the v2 and compatibility boundaries and we should only introduce a v3 if we really must. So from my side, +1 to reverting the naming to keep backward compatibility. Cheers, Gyula On Fri, Dec 1, 2023 at 10:43 AM Péter Váry wrote: > Thanks Becket for your reply! > > *On Option 1:* > - I personally consider API inconsistencies more important, since they will > remain with us "forever", but this is up to the community. I can implement > whichever solution we decide upon. > > *Option 2:* > - I don't think this specific issue merits a rewrite, but if we decide to > change our approach, then it's a different story. > > *Evolvability:* > This discussion reminds me of a similar discussion on FLIP-372 [1], where > we are trying to decide if we should use mixin interfaces, or use interface > inheritance. > With the mixin approach, we have a more flexible interface, but we can't > check the generic types of the interfaces/classes on compile time, or even > when we create the DAG. The first issue happens when we call the method and > fail. > The issue here is similar: > - *StatefulSink* needs a writer with a method to `*snapshotState*` > - *TwoPhaseCommittingSink* needs a writer with `*prepareCommit*` > - If there is a Sink which is stateful and needs to commit, then it needs > both of these methods. > > If we use the mixin solution here, we lose the possibility to check the > types in compile time. We could do the type check in runtime using ` > *instanceof*`, so we are better off than with the FLIP-372 example above, > but still lose any important possibility. I personally prefer the mixin > approach, but that would mean we rewrite the Sink API again - likely a > SinkV3. Are we ready to move down that path? > > Thanks, > Peter > > [1] - https://lists.apache.org/thread/344pzbrqbbb4w0sfj67km25msp7hxlyd > > On Thu, Nov 30, 2023, 14:53 Becket Qin wrote: > > > Hi folks, > > > > Sorry for replying late on the thread. > > > > For this particular FLIP, I see two solutions: > > > > Option 1: > > 1. On top of the the current status, rename > > *org.apache.flink.api.connector.sink2.InitContext *to > > *CommonInitContext (*should > > probably be package private*)*. > > 2. Change the name *WriterInitContext* back to *InitContext, *and revert > > the deprecation. We can change the parameter name to writerContext if we > > want to. > > Admittedly, this does not have full symmetric naming of the InitContexts > - > > we will have CommonInitContext / InitContext / CommitterInitContext > instead > > of InitContext / WriterInitContext / CommitterInitContext. However, the > > naming seems clear without much confusion. Personally, I can live with > > that, treating the class InitContext as a non-ideal legacy class name > > without much material harm. > > > > Option 2: > > Theoretically speaking, if we really want to reach the perfect state > while > > being backwards compatible, we can create a brand new set of Sink > > interfaces and deprecate the old ones. But I feel this is an overkill > here. > > > > The solution to this particular issue aside, the evolvability of the > > current interface hierarchy seems a more fundamental issue and worries me > > more. I haven't completely thought it through, but there are two > noticeable > > differences between the interface design principles between Source and > > Sink. > > 1. Source uses decorative interfaces. For example, we have a > > SupportsFilterPushdown interface, instead of a subclass of > > FilterableSource. This seems provides better flexibility. > > 2. Source tends to have a more coarse-grained interface. For example, > > SourceReader always has the methods of snapshotState(), > > notifyCheckpointComplete(). Even if they may not be always required, we > do > > not separate them into different interfaces. > > My hunch is that if we follow similar approach as Source, the > evolvability > > might be better. If we want to do this, we'd better to do it before 2.0. > > What do you think? > > > > Process wise, > > - I agree that if there is a change to the passed FLIP during > > implementation, it should be brought back to the mailing list. > > - There might be value for the connector nightly build to depend on the > > latest snapshot of the same Flink major version. It helps catching > > unexpected
Re: [DISCUSS] Resolve diamond inheritance of Sink.createWriter
Thanks Becket for your reply! *On Option 1:* - I personally consider API inconsistencies more important, since they will remain with us "forever", but this is up to the community. I can implement whichever solution we decide upon. *Option 2:* - I don't think this specific issue merits a rewrite, but if we decide to change our approach, then it's a different story. *Evolvability:* This discussion reminds me of a similar discussion on FLIP-372 [1], where we are trying to decide if we should use mixin interfaces, or use interface inheritance. With the mixin approach, we have a more flexible interface, but we can't check the generic types of the interfaces/classes on compile time, or even when we create the DAG. The first issue happens when we call the method and fail. The issue here is similar: - *StatefulSink* needs a writer with a method to `*snapshotState*` - *TwoPhaseCommittingSink* needs a writer with `*prepareCommit*` - If there is a Sink which is stateful and needs to commit, then it needs both of these methods. If we use the mixin solution here, we lose the possibility to check the types in compile time. We could do the type check in runtime using ` *instanceof*`, so we are better off than with the FLIP-372 example above, but still lose any important possibility. I personally prefer the mixin approach, but that would mean we rewrite the Sink API again - likely a SinkV3. Are we ready to move down that path? Thanks, Peter [1] - https://lists.apache.org/thread/344pzbrqbbb4w0sfj67km25msp7hxlyd On Thu, Nov 30, 2023, 14:53 Becket Qin wrote: > Hi folks, > > Sorry for replying late on the thread. > > For this particular FLIP, I see two solutions: > > Option 1: > 1. On top of the the current status, rename > *org.apache.flink.api.connector.sink2.InitContext *to > *CommonInitContext (*should > probably be package private*)*. > 2. Change the name *WriterInitContext* back to *InitContext, *and revert > the deprecation. We can change the parameter name to writerContext if we > want to. > Admittedly, this does not have full symmetric naming of the InitContexts - > we will have CommonInitContext / InitContext / CommitterInitContext instead > of InitContext / WriterInitContext / CommitterInitContext. However, the > naming seems clear without much confusion. Personally, I can live with > that, treating the class InitContext as a non-ideal legacy class name > without much material harm. > > Option 2: > Theoretically speaking, if we really want to reach the perfect state while > being backwards compatible, we can create a brand new set of Sink > interfaces and deprecate the old ones. But I feel this is an overkill here. > > The solution to this particular issue aside, the evolvability of the > current interface hierarchy seems a more fundamental issue and worries me > more. I haven't completely thought it through, but there are two noticeable > differences between the interface design principles between Source and > Sink. > 1. Source uses decorative interfaces. For example, we have a > SupportsFilterPushdown interface, instead of a subclass of > FilterableSource. This seems provides better flexibility. > 2. Source tends to have a more coarse-grained interface. For example, > SourceReader always has the methods of snapshotState(), > notifyCheckpointComplete(). Even if they may not be always required, we do > not separate them into different interfaces. > My hunch is that if we follow similar approach as Source, the evolvability > might be better. If we want to do this, we'd better to do it before 2.0. > What do you think? > > Process wise, > - I agree that if there is a change to the passed FLIP during > implementation, it should be brought back to the mailing list. > - There might be value for the connector nightly build to depend on the > latest snapshot of the same Flink major version. It helps catching > unexpected breaking changes sooner. > - I'll update the website to reflect the latest API stability policy. > Apologies for the confusion caused by the stale doc. > > Thanks, > > Jiangjie (Becket) Qin > > > > On Wed, Nov 29, 2023 at 10:55 PM Márton Balassi > wrote: > > > Thanks, Martijn and Peter. > > > > In terms of the concrete issue: > > > >- I am following up with the author of FLIP-321 [1] (Becket) to update > >the docs [2] to reflect the right state. > >- I see two reasonable approaches in terms of proceeding with the > >specific changeset: > > > > > >1. We allow the exception from FLIP-321 for this change and let the > > PublicEvolving API change happen between Flink 1.18 and 1.19, which > > is > > consistent with current state of the relevant documentation. [2] > > We commit > > to helping the connector repos make the necessary (one liner) > > changes. > > 2. We revert back to the original implementation plan as explicitly > > voted on in FLIP-371 [3]. That has no API breaking changes. > > However we end > > up with an inconsistently named API
Re: [DISCUSS] Resolve diamond inheritance of Sink.createWriter
Hi folks, Sorry for replying late on the thread. For this particular FLIP, I see two solutions: Option 1: 1. On top of the the current status, rename *org.apache.flink.api.connector.sink2.InitContext *to *CommonInitContext (*should probably be package private*)*. 2. Change the name *WriterInitContext* back to *InitContext, *and revert the deprecation. We can change the parameter name to writerContext if we want to. Admittedly, this does not have full symmetric naming of the InitContexts - we will have CommonInitContext / InitContext / CommitterInitContext instead of InitContext / WriterInitContext / CommitterInitContext. However, the naming seems clear without much confusion. Personally, I can live with that, treating the class InitContext as a non-ideal legacy class name without much material harm. Option 2: Theoretically speaking, if we really want to reach the perfect state while being backwards compatible, we can create a brand new set of Sink interfaces and deprecate the old ones. But I feel this is an overkill here. The solution to this particular issue aside, the evolvability of the current interface hierarchy seems a more fundamental issue and worries me more. I haven't completely thought it through, but there are two noticeable differences between the interface design principles between Source and Sink. 1. Source uses decorative interfaces. For example, we have a SupportsFilterPushdown interface, instead of a subclass of FilterableSource. This seems provides better flexibility. 2. Source tends to have a more coarse-grained interface. For example, SourceReader always has the methods of snapshotState(), notifyCheckpointComplete(). Even if they may not be always required, we do not separate them into different interfaces. My hunch is that if we follow similar approach as Source, the evolvability might be better. If we want to do this, we'd better to do it before 2.0. What do you think? Process wise, - I agree that if there is a change to the passed FLIP during implementation, it should be brought back to the mailing list. - There might be value for the connector nightly build to depend on the latest snapshot of the same Flink major version. It helps catching unexpected breaking changes sooner. - I'll update the website to reflect the latest API stability policy. Apologies for the confusion caused by the stale doc. Thanks, Jiangjie (Becket) Qin On Wed, Nov 29, 2023 at 10:55 PM Márton Balassi wrote: > Thanks, Martijn and Peter. > > In terms of the concrete issue: > >- I am following up with the author of FLIP-321 [1] (Becket) to update >the docs [2] to reflect the right state. >- I see two reasonable approaches in terms of proceeding with the >specific changeset: > > >1. We allow the exception from FLIP-321 for this change and let the > PublicEvolving API change happen between Flink 1.18 and 1.19, which > is > consistent with current state of the relevant documentation. [2] > We commit > to helping the connector repos make the necessary (one liner) > changes. > 2. We revert back to the original implementation plan as explicitly > voted on in FLIP-371 [3]. That has no API breaking changes. > However we end > up with an inconsistently named API with duplicated internal > methods. Peter > has also discovered additional bad patterns during his work in > FLIP-372 > [3], the total of these changes could be handled in a separate FLIP > that > would do multiple PublicEvolving breaking changes to clean up the > API. > > In terms of the general issues: > >- I agree that if a PR review of an accepted FLIP newly introduces a >breaking API change that warrants an update to the mailing list > discussion >and possibly even a new vote. >- I agree with the general sentiment of FLIP-321 to provide stronger API >guarantees with the minor note that if we have changes in mind we should >prioritize them now such that they can be validated by Flink 2.0. >- I agree that ideally the connector repos should build against the >latest release and not the master branch. > > [1] > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-321%3A+Introduce+an+API+deprecation+process > [2] > > https://nightlies.apache.org/flink/flink-docs-master/docs/ops/upgrading/#api-compatibility-guarantees > [3] > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-371%3A+Provide+initialization+context+for+Committer+creation+in+TwoPhaseCommittingSink > [4] > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-372%3A+Allow+TwoPhaseCommittingSink+WithPreCommitTopology+to+alter+the+type+of+the+Committable > > Best, > Marton > > On Mon, Nov 27, 2023 at 7:23 PM Péter Váry > wrote: > > > I think we should try to separate the discussion in a few different > topics: > > > >- Concrete issue > > - How to solve this problem in 1.19 and wrt the affected > createWriter > > interface > > - Update the
Re: [DISCUSS] Resolve diamond inheritance of Sink.createWriter
Thanks, Martijn and Peter. In terms of the concrete issue: - I am following up with the author of FLIP-321 [1] (Becket) to update the docs [2] to reflect the right state. - I see two reasonable approaches in terms of proceeding with the specific changeset: 1. We allow the exception from FLIP-321 for this change and let the PublicEvolving API change happen between Flink 1.18 and 1.19, which is consistent with current state of the relevant documentation. [2] We commit to helping the connector repos make the necessary (one liner) changes. 2. We revert back to the original implementation plan as explicitly voted on in FLIP-371 [3]. That has no API breaking changes. However we end up with an inconsistently named API with duplicated internal methods. Peter has also discovered additional bad patterns during his work in FLIP-372 [3], the total of these changes could be handled in a separate FLIP that would do multiple PublicEvolving breaking changes to clean up the API. In terms of the general issues: - I agree that if a PR review of an accepted FLIP newly introduces a breaking API change that warrants an update to the mailing list discussion and possibly even a new vote. - I agree with the general sentiment of FLIP-321 to provide stronger API guarantees with the minor note that if we have changes in mind we should prioritize them now such that they can be validated by Flink 2.0. - I agree that ideally the connector repos should build against the latest release and not the master branch. [1] https://cwiki.apache.org/confluence/display/FLINK/FLIP-321%3A+Introduce+an+API+deprecation+process [2] https://nightlies.apache.org/flink/flink-docs-master/docs/ops/upgrading/#api-compatibility-guarantees [3] https://cwiki.apache.org/confluence/display/FLINK/FLIP-371%3A+Provide+initialization+context+for+Committer+creation+in+TwoPhaseCommittingSink [4] https://cwiki.apache.org/confluence/display/FLINK/FLIP-372%3A+Allow+TwoPhaseCommittingSink+WithPreCommitTopology+to+alter+the+type+of+the+Committable Best, Marton On Mon, Nov 27, 2023 at 7:23 PM Péter Váry wrote: > I think we should try to separate the discussion in a few different topics: > >- Concrete issue > - How to solve this problem in 1.19 and wrt the affected createWriter > interface > - Update the documentation [1], so FLIP-321 is visible for every > contributor >- Generic issue > - API stability > - Connector dependencies > > > *CreateWriter interface* > The change on the createWriter is not strictly required for the > functionality defined by the requirements on the FLIP. > If the only goal is only to have a backward compatible API, we can simply > create a separate `*CommitterInitContext*` object and do not touch the > writer `*InitContext*`, like it was done in the original PR [2]. > The issue is that this would result in an implementation which has > duplicated methods/implementations (internal issue only), and has > inconsistent naming (issue for external users). > > If we want to create an API which is consistent (and I agree with the > reviewer's comments), then we need to rename the parameter type ( > *WriterInitContext*) for the createWriter method. > I have tried to keep the backward compatibility with creating a new method > and providing a default implementation for this new method which would call > the original method after converting the WriterInitContext to InitContext. > > This is failed because the following details: > >- *org.apache.flink.api.connector.sink2.Sink* defines > `*SinkWriter >createWriter(InitContext context)`* >- *org.apache.flink.api.connector.sink2.StatefulSink* narrows it > down to *`StatefulSinkWriterWriterStateT> createWriter(InitContext context)`* >- *org.apache.flink.api.connector.sink2.TwoPhaseCommittingSink* narrows >it down to *`PrecommittingSinkWriter >createWriter(WriterInitContext context)`* >- > > > *org.apache.flink.streaming.runtime.operators.sink.TestSinkV2.TestStatefulSinkV2* >implements *StatefulSink* and *TwoPhaseCommittingSink* too > > *TestStatefulSinkV2* is a good example where we can not achieve backward > compatibility, since the the compiler will fail with unrelated default > methods [3] > > I am open for any suggestions how to move to the new API, and keep the > backward compatibility. If we do not find a way to keep backward > compatibility, and we decide that we would like to honour FLIP-321, then we > can reverting to the original solution and keep only the changes for the ` > *createCommitter*` method. > > *Update the documentation* > I have not found only one place in the docs [1], where we talk about the > compatibility guarantees. > Based FLIP-321 and the result of the discussion here, we should update this > page. > > *API stability* > I agree with the general sentiment of FLIP-321 to keep the changes backward >
Re: [DISCUSS] Resolve diamond inheritance of Sink.createWriter
I think we should try to separate the discussion in a few different topics: - Concrete issue - How to solve this problem in 1.19 and wrt the affected createWriter interface - Update the documentation [1], so FLIP-321 is visible for every contributor - Generic issue - API stability - Connector dependencies *CreateWriter interface* The change on the createWriter is not strictly required for the functionality defined by the requirements on the FLIP. If the only goal is only to have a backward compatible API, we can simply create a separate `*CommitterInitContext*` object and do not touch the writer `*InitContext*`, like it was done in the original PR [2]. The issue is that this would result in an implementation which has duplicated methods/implementations (internal issue only), and has inconsistent naming (issue for external users). If we want to create an API which is consistent (and I agree with the reviewer's comments), then we need to rename the parameter type ( *WriterInitContext*) for the createWriter method. I have tried to keep the backward compatibility with creating a new method and providing a default implementation for this new method which would call the original method after converting the WriterInitContext to InitContext. This is failed because the following details: - *org.apache.flink.api.connector.sink2.Sink* defines `*SinkWriter createWriter(InitContext context)`* - *org.apache.flink.api.connector.sink2.StatefulSink* narrows it down to *`StatefulSinkWriter createWriter(InitContext context)`* - *org.apache.flink.api.connector.sink2.TwoPhaseCommittingSink* narrows it down to *`PrecommittingSinkWriter createWriter(WriterInitContext context)`* - *org.apache.flink.streaming.runtime.operators.sink.TestSinkV2.TestStatefulSinkV2* implements *StatefulSink* and *TwoPhaseCommittingSink* too *TestStatefulSinkV2* is a good example where we can not achieve backward compatibility, since the the compiler will fail with unrelated default methods [3] I am open for any suggestions how to move to the new API, and keep the backward compatibility. If we do not find a way to keep backward compatibility, and we decide that we would like to honour FLIP-321, then we can reverting to the original solution and keep only the changes for the ` *createCommitter*` method. *Update the documentation* I have not found only one place in the docs [1], where we talk about the compatibility guarantees. Based FLIP-321 and the result of the discussion here, we should update this page. *API stability* I agree with the general sentiment of FLIP-321 to keep the changes backward compatible as much as possible. But the issue above highlights that there could be situations where it is not possible to achieve backward compatibility. Probably we should provide exceptions to handle this kind of situations - minimally for PublicEvolving interfaces. After we agree on long term goals - allowing exceptions or to be more lenient on backward compatibility guarantees, or sticking to FLIP-321 by the letter - we could discuss how to apply it to the current situation. *Connector dependencies* I think it is generally a good practice to depend on the stable version of Flink (or any other downstream project). This is how we do it in Iceberg, and how it was implemented in the Kafka connector as well. This would result in more stable connector builds. The only issue I see, that the situations like this would take longer to surface, but I fully expect us to get better at compatibility after we wetted the process. [1] - https://nightlies.apache.org/flink/flink-docs-master/docs/ops/upgrading/#api-compatibility-guarantees [2] - https://github.com/apache/flink/pull/23555/commits/2b9adeb20e55c33a623115efa97d3149c11e9ca4 [3] - https://github.com/apache/flink/pull/23555#discussion_r1371740397 Martijn Visser ezt írta (időpont: 2023. nov. 27., H, 11:21): > Hi all, > > I'm opening this discussion thread to bring a discussion that's > happening on a completed Jira ticket back to the mailing list [1] > > In summary: > > * There was a discussion and a vote on FLIP-371 [2] > * During implementation, it was determined that there's a diamond > inheritance problem on the Sink.createWriter method, making a > backwards compatible change hard/impossible (I think this is where the > main discussion point actually is) [3] > * The PR was merged, causing a backwards incompatible change without a > discussion on the Dev mailing list > > I think that in hindsight, even though there was a FLIP on this topic, > the finding of the diamond inheritance issue should have been brought > back to the Dev mailing list in order to agree on how to resolve it. > Since 1.19 is still under way, we still have time to fix this. > > I think there's two things we can improve: > > 1) Next time during implementation of a FLIP/PR which involves a > non-backward compatible change of an API that wasn't accounted for, >
[DISCUSS] Resolve diamond inheritance of Sink.createWriter
Hi all, I'm opening this discussion thread to bring a discussion that's happening on a completed Jira ticket back to the mailing list [1] In summary: * There was a discussion and a vote on FLIP-371 [2] * During implementation, it was determined that there's a diamond inheritance problem on the Sink.createWriter method, making a backwards compatible change hard/impossible (I think this is where the main discussion point actually is) [3] * The PR was merged, causing a backwards incompatible change without a discussion on the Dev mailing list I think that in hindsight, even though there was a FLIP on this topic, the finding of the diamond inheritance issue should have been brought back to the Dev mailing list in order to agree on how to resolve it. Since 1.19 is still under way, we still have time to fix this. I think there's two things we can improve: 1) Next time during implementation of a FLIP/PR which involves a non-backward compatible change of an API that wasn't accounted for, the discussion should be brought back to the Dev mailing list. I think we can just add that to the FLIP bylaws. 2) How do we actually resolve the problem: is there anyone who has an idea on how we could introduce the proposed change while maintaining backwards compatibility, or do we agree that while this is an non desired situation, there is no better alternative unfortunately? Best regards, Martijn [1] https://issues.apache.org/jira/browse/FLINK-25857 [2] https://cwiki.apache.org/confluence/display/FLINK/FLIP-371%3A+Provide+initialization+context+for+Committer+creation+in+TwoPhaseCommittingSink [3] https://github.com/apache/flink/pull/23555#discussion_r1371740397
