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
>
