Hi all, I wanted to get back on a couple of comments and have a proposal at the end for the next steps:
@Steven Wu If I look back at the original discussion of FLIP-191 and also the thread that you're referring to, it appears from the discussion with Yun Gao that the solution was in near-sight, but just not finished. Perhaps it needs to be restarted once more so it can be brought to a closure. Also when I looked back at the original introduction of SinkV2, there was FLINK-25726 [1] which looks like it was built specifically for Iceberg and Delta Sink? @Jing > All potential unstable methods coming from SinkV2 interfaces will be kept marked as @internal. This is a situation that I think should be avoided: if it's unstable, it should be marked as @Experimental. Connectors should not need to rely on @Internal interfaces; if they are needed by a connector, a maintainer should create a proposal to expose. On the production readiness of SinkV2: @Dong > And Yuxia mentioned earlier in this thread that there are bugs such as FLINK-30238 <https://issues.apache.org/jira/browse/FLINK-30238> and FLINK-29459 <https://issues.apache.org/jira/browse/FLINK-29459> , which makes it hard to use SinkV2 properly in production. @Jark > The follow-up step should be trying the new APIs in externalized connectors and giving users the confidence to migrate. I think that is not a fair assessment of the situation. Given that Kafka, Elasticsearch, FileSink and everything that uses the AsyncSink is on SinkV2 since Flink 1.15 and the popularity of these connectors, they have definitely already proven themselves. That's not to downplay that bugs have no impact and shouldn't be resolved, but I don't think it's fair to state that SinkV2 is not stable or usable in production. Coming back to the original topic of this thread, my proposal would be that in Flink 1.17: 1. SinkV2 gets promoted from @PublicEvolving to @Public, which also means: a. KafkaSink gets promoted from @PublicEvolving to @Public (which solves the weird thing that FlinkKafkaProducer is already marked as deprecated since Flink 1.14 and points to KafkaSink as a successor) b. FileSink gets promoted from @Experimental to @PublicEvolving (note: this has been marked as @Experimental since Flink 1.12, so one could argue that this needs to be bumped to @Public too. The reason why I didn't do it is because of Yuxia's remark that FileSystemTableSink is still using SinkFunction) c. ElasticsearchSink gets promoted from @PublicEvolving to @Public (has been externalized so needs to happen there) d. The AWS connectors (Firehose, Kinesis) get promoted from @PublicEvolving to @Public (has been externalized so needs to happen there) e. It's up to the maintainers for DynamoDB and Opensearch to decide this themselves, given that I believe they've only been introduced recently and support only Flink 1.16. 2. For the next Flink release, the community needs to establish what are features that are currently not available in SinkV2 (e.g. FLIP-287), if such a feature should be made available in SinkV2 (in case it's a feature which currently resides in SinkFunction, while from new insights we think it should now be solved differently) and then get these implemented asap. Best regards, Martijn [1] https://issues.apache.org/jira/browse/FLINK-25726 On Tue, Feb 7, 2023 at 5:11 AM Steven Wu <stevenz...@gmail.com> wrote: > > 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? > > Just want to reiterate my earlier question. What is the migration path for > the Iceberg sink? > > [1] https://lists.apache.org/thread/82bgvlton9olb591bfg2djv0cshj1bxj > > On Mon, Feb 6, 2023 at 6:22 PM Jark Wu <imj...@gmail.com> wrote: > > > Hi Konstantin, > > > > I totally agree with making SinkV2 @Public. I just have concerns about > > deprecating SinkFunction at this point. Dong Lin has raised the blocker > > issues of migration multiple times in this thread which I think we should > > address first. I don't know why we rush to deprecate SinkFunction while > > SourceFunction is still public, but the new Source API is much more > stable > > and production-ready than SinkV2. > > > > Iceberg community raised concerns[1] about the workability and stability > of > > Flink connector APIs. > > > > We are hoping any issues with the APIs for Iceberg connector will surface > > > sooner and get more attention from the Flink community when the > connector > > > is within Flink umbrella rather than in Iceberg repo. > > > > > > The connector externalizing is a big step for building a mechanism to > > guarantee Flink connector API is stable and workable. The follow-up step > > should be trying the new APIs in externalized connectors and giving users > > the confidence to migrate. > > > > > > Best, > > Jark > > > > [1]: https://lists.apache.org/thread/r5zqnkt01x2c611brkjmxxnt3bfcgl1b > > > > On Tue, 7 Feb 2023 at 09:53, yuxia <luoyu...@alumni.sjtu.edu.cn> wrote: > > > > > Hi Konstantin, > > > Just FYI, the FileSystemTableSink are still using SinkFunction. > > > > > > Best regards, > > > Yuxia > > > > > > ----- 原始邮件 ----- > > > 发件人: "Dong Lin" <lindon...@gmail.com> > > > 收件人: "dev" <dev@flink.apache.org> > > > 抄送: "Jing Ge" <j...@ververica.com>, "Yun Tang" <myas...@live.com> > > > 发送时间: 星期二, 2023年 2 月 07日 上午 9:41:07 > > > 主题: Re: [DISCUSS] Promote SinkV2 to @Public and deprecate SinkFunction > > > > > > Hi Konstantin, > > > > > > Thanks for the reply. Please see my comments inline. > > > > > > On Mon, Feb 6, 2023 at 9:48 PM Konstantin Knauf <kna...@apache.org> > > wrote: > > > > > > > Hi Steven, > > > > > > > > Sink is already deprecated. It was deprecated at the moment where we > > > > introduced SinkV2. > > > > > > > > Hi Jark, Hi Dong, > > > > > > > > My understanding is the SinkV2 is a workable interface. The most > > > important > > > > connectors have been migrated (Kafka, Filesystem) and more connectors > > > > (OpenSearch, ElasticSearch, Kinesis) use it successfully. To make > > SinkV2 > > > > public, it does not need to have all possible functionality. Public > > APIs > > > > can be extended. That's what we do all the time. There will also > always > > > be > > > > bugs. So, these points can not be categorical blockers to promote the > > > API. > > > > > > > > > > Right. I believe we all agree that we make SinkV2 @PublicEvolving. The > > > concern here is whether we can mark SinkFunction as deprecated at this > > > point. > > > > > > > > > > > > > > What are the specific issues/tickets that are blocking us? Can we in > > your > > > > > > > > > > For example, Lijie mentioned earlier in this thread that according to > > > FLIP-287, > > > currently the Sink.InitContext still lacks some necessary information > to > > > migrate existing connectors to new sinks. This could be a blocker issue > > > since this is related to the SinkV2 API design. > > > > > > And Yuxia mentioned earlier in this thread that there are bugs such as > > > FLINK-30238 <https://issues.apache.org/jira/browse/FLINK-30238> and > > > FLINK-29459 <https://issues.apache.org/jira/browse/FLINK-29459> , > which > > > makes it hard to use SinkV2 properly in production. It seems that > these > > > two bugs are created months ago and are still unresolved or even > > > unassigned. This looks like a clear signal that SinkV2 is not being > > > actively maintained and used in production. > > > > > > > > > > opinion only deprecate it when every single connector in Apache Flink > > is > > > > migrated already? > > > > > > > > > > Technically this is not a hard requirement. But I would suggest that we > > > should migrate all existing connectors so that we eat our own dogfood > and > > > prove to users that SinkV2 is ready for use in production. > > > > > > > > > > In my opinion it is the time to ask users to the migrate their > > > connectors. > > > > More importantly, @Deprecated would signal users not to build new > > > > connectors on SinkFunction. I would arque its also very misleading to > > > users > > > > to not @Deprecated SinkFunction given that is clearly will be > > deprecated. > > > > > > > > > > > > > > > > Cheers, > > > > > > > > Konstantin > > > > > > > > > > > > Am Mo., 6. Feb. 2023 um 13:26 Uhr schrieb Jark Wu <imj...@gmail.com > >: > > > > > > > > > I agree with Dong Lin. > > > > > > > > > > Oracle explains how to use Deprecate API [1]: > > > > > > > > > > You are strongly recommended to use the Javadoc @deprecated tag > with > > > > > > appropriate comments explaining how to use the new API. This > > ensures > > > > > > developers will *have a workable migration path from the old API > to > > > the > > > > > > new API*. > > > > > > > > > > > > > > > From a user's perspective, the workable migration path is very > > > important. > > > > > Otherwise, it blurs the semantics of API deprecation. The Flink > API's > > > > > compatibility and stability issues in the past left a bad > impression > > on > > > > the > > > > > downstream projects. We should be careful when changing and > > deprecating > > > > > APIs, especially when there are known migration gaps. I think it's > a > > > good > > > > > idea to migrate Flink-owned connectors before marking old API > > > deprecated. > > > > > This ensures downstream projects can migrate to new APIs smoothly. > > > > > > > > > > Best, > > > > > Jark > > > > > > > > > > [1]: > > > > > > > > > > > > > > > > > > > > https://docs.oracle.com/javase/8/docs/technotes/guides/javadoc/deprecation/deprecation.html > > > > > > > > > > On Mon, 6 Feb 2023 at 10:01, Steven Wu <stevenz...@gmail.com> > wrote: > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > https://twitter.com/snntrable > > > > https://github.com/knaufk > > > > > > > > > >
