Regarding the discussion on global committer [1] for sinks with global transactions, there is no consensus on solving that problem in SinkV2. Will it require any breaking change in SinkV2?
Also will SinkV1 be deprecated too? or it should happen sometime after SinkFunction deprecation? [1] https://lists.apache.org/thread/82bgvlton9olb591bfg2djv0cshj1bxj On Sun, Feb 5, 2023 at 2:14 AM Dong Lin <lindon...@gmail.com> wrote: > Hi Konstantin, > > Thanks for the comment! Please see my comment inline. > > Cheers, > Dong > > On Sat, Feb 4, 2023 at 2:06 AM Konstantin Knauf <kna...@apache.org> wrote: > > > Hi everyone, > > > > sorry for joining the discussion late. > > > > 1) Is there an option to deprecate SinkFunction in Flink 1.17 while > leaving > > SinkV2 @PublicEvolving in Flink 1.17. We then aim to make SinkV2 @Public > in > > and remove SinkFunction in Flink 1.18. @PublicEvolving are intended for > > public use. So, I don't see it as a blocker for deprecating SinkFunction > > that we have to make SinkV2 @PublicEvovling. For reference this is the > > description of @PublicEvovling: > > > > /** > > * Annotation to mark classes and methods for public use, but with > > evolving interfaces. > > * > > * <p>Classes and methods with this annotation are intended for public > > use and have stable behavior. > > * However, their interfaces and signatures are not considered to be > > stable and might be changed > > * across versions. > > * > > * <p>This annotation also excludes methods and classes with evolving > > interfaces / signatures within > > * classes annotated with {@link Public}. > > */ > > > > > > Marking SinkFunction @Deprecated would already single everyone to move to > > SinkV2, which we as a community, I believe, have a strong interest in. > Its > > > > Yes, I also believe we all have this strong interest. I just hope that this > can be done in the best possible way that does not confuse users. > > I probably still have the same concern regarding its impact on users: if we > mark an API as deprecated, it effectively means the users of this API > should start to migrate to another API (e.g. SinkV2) and we might remove > this API in the future. However, given that we know there are known > problems preventing users from doing so, it seems that we are not ready to > send this message to users right. > > If I understand correctly, I guess you are suggesting that by marking > SinkFunction as deprecated, we can put higher pressure on Flink > contributors to update the existing Flink codebase to improve and use > SinkV2. > > I am not sure this is the right way to use @deprecated, which has a > particular meaning for its users rather than contributors. And I am also > not sure we can even pressure contributors of an open-source project into > developing a feature (e.g. migrate all existing SinkFunction subclasses to > SinkV2). IMO, the typical way is for the contributor with interest/time to > work on the feature, or talk to other contributors whether they are willing > to collaborate/work on this, rather than pressuring other contributors into > working on this. > > > almost comical how long the transition from SourceFurnction/SinkFunction to > > Source/Sink takes us. At the same time, we leave ourselves the option to > to > > make small changes to SinkV2 if any problems arise during the migration > of > > these connector. > > > > I think, we have a bit of a chicken/egg problem here. The pressure for > > > > Similar to the reason described above, I am not sure we have a chicken/egg > problem here. The issue here is that SinkV2 is not ready and we have a lot > of existing SinkFunction that is not migrated by ourselves. We (Flink > contributors) probably do not need to mark SinkFunction as deprecated in > order to address these issues in our own codebase. > > > users and contributors is not high enough to move away from SinkFunction as > > long as its not deprecated, but at the same time we need people to > migrate > > their connectors to see if there are any gaps in SinkV2. I believe, the > > combination proposed above could bridge this problem. > > > > 2) I don't understand the argument of waiting until some of the > > implementations are @Public. How can we make the implementations of the > > SinkV2 API @Public without making SinkV2 @Public? All public methods of > > SinkV2 are part of every implementation. So to me it actually seems to be > > opposite: in order to make any of the implementation @Public we first > need > > to make the API @Public. > > > > Yeah I also agree with you. > > > > > > Cheers, > > > > Konstantin > > > > Am Mo., 30. Jan. 2023 um 13:18 Uhr schrieb Dong Lin <lindon...@gmail.com > >: > > > > > Hi Martijn, > > > > > > Thanks for driving this effort to clean-up the Flink codebase! > > > > > > I like the idea to cleanup Flink codebase to avoid having two Sinks. On > > the > > > other hand, I also thing the concern mentioned by Jing makes sense. In > > > addition to thinking in terms of the rule proposed in FLIP-197 > > > < > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-197%3A+API+stability+graduation+process > > > > > > > (which > > > seems to focus mostly on the Flink developers' perspective), it might > be > > > useful to also think about the story from users' perspective and make > > sure > > > their concerns can be addressed. > > > > > > Typically, by marking an API as deprecated, we are effectively telling > > > users *they should start to migrate to the new API ASAP and we reserve > > the > > > right to remove this API completely in the 1-2 releases*. Then it might > > be > > > reasonable for users to ask questions such as: > > > > > > 1) Does SinkV2 public API provides all the functionalities needed to > > > migrate my existing code from subclassing SinkFunction to subclassing > > > SinkV2? > > > > > > 2) Is the amount of migration work reasonable? If yes, why is a class > > such > > > as HBaseSinkFunction in Flink's own codebase still depending on > > > SinkFunction? Maybe Flink developers should eat their own dog food and > > > migrate (or deprecate) those classes in the Flink codebase first? > > > > > > Based on the discussion in this thread, I am not sure we have good > > answers > > > to those questions yet. For the 1st question above, the answer is *no* > > > because we already know that the SinkV2 is currently not able to > support > > > migration for JdbcSink. For the 2nd question above, we know there are > > many > > > non-deprecated class in Flink codebase that are still depending on > > SinkV2. > > > It is probably not nice to users if we tell them to migrate while we > know > > > there are existing issues that can prevent them from doing so easily. > > > > > > In order to move forward to deprecate SinkV2, I think it will be super > > > useful to first migrate all the connectors managed by Flink community > > > (including all externalized connectors) to use SinkV2. This work won't > be > > > wasted since we need to do this anyway. And it will also give us a > chance > > > to validate the capabilities of SinkV2 and fix bugs by ourselves as > much > > as > > > possible. > > > > > > What do you think? > > > > > > Best Regards, > > > Dong > > > > > > > > > On Wed, Jan 18, 2023 at 6:52 PM Martijn Visser < > martijnvis...@apache.org > > > > > > wrote: > > > > > > > Hi all, > > > > > > > > While discussing FLIP-281 [1] the discussion also turned to the > > > > SinkFunction and the SinkV2 API. For a broader discussion I'm opening > > up > > > a > > > > separate discussion thread. > > > > > > > > As Yun Tang has mentioned in that discussion thread, it would be a > good > > > > time to deprecate the SinkFunction to avoid the need to introduce new > > > > functions towards (to be) deprecated APIs. Jing rightfully mentioned > > that > > > > it would be confusing to deprecate the SinkFunction if its successor > is > > > not > > > > yet marked as @Public (it's currently @PublicEvolving). > > > > > > > > My proposal would be to promote the SinkV2 API to @public in Flink > 1.17 > > > and > > > > mark the SinkFunction as @deprecated in Flink 1.17 > > > > > > > > The original Sink interface was introduced in Flink 1.12 with > FLIP-143 > > > [2] > > > > and extended with FLIP-177 in Flink 1.14 [3] and has been improved on > > > > further as Sink V2 via FLIP-191 in Flink 1.15 [4]. > > > > > > > > Looking at the API stability graduation process [5], the fact that > Sink > > > V2 > > > > was introduced in Flink 1.15 would mean that we could warrant a > > promotion > > > > to @public already (given that there have been two releases with 1.15 > > and > > > > 1.16 where it was introduced). Combined with the fact that SinkV2 has > > > been > > > > the result of iteration over the introduction of the original Sink > API > > > > since Flink 1.12, I would argue that the promotion is overdue. > > > > > > > > If we promote the Sink API to @public, I think we should also > > immediately > > > > mark the SinkFunction as @deprecated. > > > > > > > > Looking forward to your thoughts. > > > > > > > > Best regards, > > > > > > > > Martijn > > > > > > > > > > > > [1] https://lists.apache.org/thread/l05m6cf8fwkkbpnjtzbg9l2lo40oxzd1 > > > > [2] > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-143%3A+Unified+Sink+API > > > > [3] > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-177%3A+Extend+Sink+API > > > > [4] > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-191%3A+Extend+unified+Sink+interface+to+support+small+file+compaction > > > > [5] > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-197%3A+API+stability+graduation+process > > > > > > > > > > > > > -- > > https://twitter.com/snntrable > > https://github.com/knaufk > > >
