Hi everyone, It’s been a while since this topic was last discussed, but nevertheless, it still remains very desirable to figure out a clear path towards making SinkV2 @Public.
There’s a new thread [1] that has a pretty good description on missing features in SinkV2 from the Iceberg connector’s perspective, which prevents them from migrating - anything related to those new requirements, let's discuss there. Nevertheless, I think we should also revive and reuse this thread to reach consensus / closure on all concerns already brought up here. It’s quite a long thread where a lot of various concerns were brought up, but I’d start by addressing two very specific ones: FLIP-287 [2] and FLINK-30238 [3] First of all, FLIP-287 has been approved and merged already, and will be released with 1.18.0. So, connector migrations that were waiting on this should hopefully be unblocked after the release. So this seems to no longer be a concern - let’s see things through with those connectors actually being migrated. FLINK-30238 is sort of a confusing one, and I do believe it is (partially) a false alarm. After looking into this, the problem reported there essentially breaks down to two things: 1) TwoPhaseCommittingSink is unable to take a new savepoint after restoring from a savepoint generated via `stop-with-savepoint --drain` 2) SinkV2 sinks with a PostCommitTopology do not properly have post-commits completed after a stop-with-savepoint operation, since committed commitables are not emitted to the post-commit topology after the committer receives the end-of-input signal. My latest comment in [3] explains this in a bit more detail. I believe we can conclude that problem 1) is a non-concern - users should not restore from a job that is drained on stop-with-savepoint and cannot expect the restored job to function normally. Problem 2) remains a real issue though, and to help clear things up I think we should close FLINK-30238 in favor of a new ticket scoped to the specific PostCommitTopology problem. The other open concerns seem to mostly be around graduation criteria and process - I've yet to go through those and will follow up with a separate reply (or perhaps Martijn can help wrap up that part?). Thanks, Gordon [1] https://lists.apache.org/thread/h3kg7jcgjstpvwlhnofq093vk93ylgsn [2] https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=240880853 [3] https://issues.apache.org/jira/browse/FLINK-30238 On Mon, Feb 13, 2023 at 2:50 AM Jing Ge <j...@ververica.com.invalid> wrote: > @Martijn > > What I shared previously is the fact of the current KafkaSink. Following > your suggestion, the KafkaSink should still be marked as @Experimental for > now which will need even longer time to graduate. BTW, KafkaSink does not > depend on any @Internal interfaces. The @Internal is used for methods > coming from @PublicEvolving SinkV2 interfaces, not interfaces themself. > Thanks for bringing this topic up. Currently, there is no rule defined to > say that no @Internal is allowed for methods implemented > from @PublicEvolving interfaces. Further (off-this-topic) discussion might > be required to check if it really makes sense to define such a rule, since > some methods defined in interfaces might only be used internally, i.e. no > connector user would be aware of them. > > @Dong > > I agree with everything you said and especially can't agree more to let > developers who will own it make the decision. > > Best regards, > Jing > > > On Sun, Feb 12, 2023 at 2:53 AM Dong Lin <lindon...@gmail.com> wrote: > > > Hi Martijn, > > > > Thanks for the reply. Please see my comments inline. > > > > Regards, > > Dong > > > > On Sat, Feb 11, 2023 at 4:31 AM Martijn Visser <martijnvis...@apache.org > > > > wrote: > > > > > 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. > > > > > > > I believe there exist jobs that are currently using SinkV2 (e.g. Kafka, > > FileSink) in production. The questions are whether 1) SinkV2 is ready to > > support all features of all existing SinkFunction; and 2) whether all > those > > connectors (e.g. Kafka) are ready to support all features of their > > SinkFunction version (e.g. FlinkKafkaProducer) reliable without critical > > bugs (e.g. losing data). > > > > If we can not answer "yes" to the above two questions, then we probably > can > > not say that SinkV2 is usable in production in general, even though we > can > > still say that some source/sink based on SinkV2 is usable in production > in > > selected scenarios (e.g. maybe those jobs that do not use on savepoint). > > > > Maybe we can use FLINK-30238 > > <https://issues.apache.org/jira/browse/FLINK-30238> to explain this > point. > > The JIRA's description basically says that "there is the risk of losing > > data if we use stop-with-savepoint with SinkV2". Do you know whether this > > might potentially affect those SinkV2 (e.g. Kafka, FileSink) jobs in > > production when they are restarted with savepoint? > > > > > > > > > > > > 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: > > > > > > > I believe we have agreed to address the issues with SinkV2. Now the > > question is, how likely would the solutions require backward incompatible > > change to SinkV2. > > > > For example, do you think the following questions (mentioned earlier in > > this thread) would require backward incompatible changes? > > - Sink.InitContext still lacks some necessary information to migrate > > existing connectors to new sinks > > - Address FLINK-30238 and FLINK-29459. > > - Find the migration path for the Iceberg sink > > > > IMO, it seems safer to mark an interface as @Public when we are pretty > sure > > that there is no known issue in the near future that would require > > backward-incompatible changes to the interface. > > > > Note that the cost of making an interface @Public lies on the > developer(s) > > who owns the task of updating SinkV2 to address those known issues (e.g. > > migration). Maybe we should find the developers who will own this task > and > > let them decide whether to mark it @Public? > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > >
