You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flink.apache.org by OpenInx <op...@gmail.com> on 2021/09/28 04:33:52 UTC
The Apache Flink should pay more attention to ensuring API compatibility.
Hi Dev
We are trying to upgrade the flink version from 1.12.0 to 1.13.2 in apache
iceberg project ( https://github.com/apache/iceberg/pull/3116), but it's
not a great experience. We expect to support both flink1.12 and flink1.13
in an iceberg-flink module without using the new API of flink1.13 for
saving maintenance cost, but we find the iceberg-flink-runtime.jar built
by flink 1.13 cannot works fine in flink 1.12 clusters because of the basic
API compatibility was break when iterating flink 1.12 to flink1.13.2:
(The following are copied from the iceberg issue:
https://github.com/apache/iceberg/issues/3187#issuecomment-928755046)
Thanks for the report, @Reo-LEI ! I think this issue was introduced from
this apache flink PR (
https://github.com/apache/flink/pull/15316/files#diff-bd276ed951054125b39428ee61de103d9c7832246398f01514a574bb8e51757cR74)
and FLINK-21913 (https://issues.apache.org/jira/browse/FLINK-21913), it
just changed the returned data type from CatalogTable to
ResolvedCatalogTable without any compatibility guarantee. In this case, the
iceberg-flink-runtime jar which is compiled from apache flink 1.13 will
include the ResovledCatalogTable class inside it. Finally when we package
this jar and submit the flink job to flink 1.12, the above compatibility
issue happen.
As we all know, the DynamicTableFactory (
https://github.com/apache/flink/blob/99c2a415e9eeefafacf70762b6f54070f7911ceb/flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/DynamicTableFactory.java)
is a basic API which almost all flink connectors are built on top of it.
The breaking compatibility makes the downstream projects really hard to
deliver better compatibility to users, unless we iceberg maintain different
modules for each maintained flink version (That's not the thing that we
want to do).
The last flink upgrading work is also not a good experience (See the
discussion (https://github.com/apache/iceberg/pull/1956) and comment (
https://github.com/apache/iceberg/pull/1956#discussion_r546534299) ),
because the flink 1.12 also breaks several API that was annotated
PublicEvolving in flink 1.11.0, that becomes one of the most important
reasons leading to the conclusion that stops support flink 1.11.0 in our
apache iceberg branch ( Supporting new features [such as flip-27 unified
iceberg source/sink] that depends the API introduced in flink 1.12 is
another reason). To better support the compatibility of downstream systems
and delivering better experience to flink users, I will strongly suggest
the Apache Flink community to pay more attention to ensuring API
compatibility.
Zheng Hu (openinx)
Thanks.
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Arvid Heise <ar...@apache.org>.
Thanks for starting the discussion. I think both issues are valid concerns
that we need to tackle. I guess the biggest issue is that now it's just not
possible to write 1 connector that runs for Flink 1.13 and 1.14, so we make
it much harder for devs in the ecosystem (and our goal is to make it
easier!).
I think the only solution is to actually treat (and mark) all these
interfaces as Public and only change them in Flink 2.0. If we need to
change them, we would need to actually add a secondary (possibly internal)
interface (at least that would have helped in SourceReaderContext).
On Tue, Sep 28, 2021 at 3:20 PM Chesnay Schepler <ch...@apache.org> wrote:
> We already have such tooling via japicmp; it's just that it is only
> enabled for Public APIs.
>
> You can probably generate a report via japicmp for all
> PublicEvolging/Experimental APIs as well.
>
> On 28/09/2021 15:17, Ingo Bürk wrote:
> > Hi everyone,
> >
> > I think it would be best to support this process with tooling as much as
> > possible, because humans are bound to make mistakes. FLINK-24138[1]
> should
> > be a first step in this direction, but it wouldn't catch the cases
> > discussed here.
> > Maybe we should consider "capturing" the public API into some separate
> > file(s) and validating against that in the CI such that structural
> changes
> > to public APIs (moved classes, renamed / exchanged classes, changed
> > signatures, …) can be caught as an error. That would raise awareness for
> > any changes to public APIs and force a conscious decision.
> >
> > [1] https://issues.apache.org/jira/browse/FLINK-24138
> >
> >
> > Best
> > Ingo
> >
> > On Tue, Sep 28, 2021 at 2:49 PM Leonard Xu <xb...@gmail.com> wrote:
> >
> >>>> Not sure if this will happen in 1.15 already. We will needed automated
> >>>> compatibility tests and a well-defined list of stable API.
> >>> We are
> >>> trying to provide forward compatibility: applications using `@Public`
> >> APIs
> >>> compiled against Flink 1.12.x, should work fine in Flink 1.13.x
> >> Unfortunately, I also meet forward compatibility issue, when I do the
> >> release 1.14 check, I try to use mysql-cdc connector[1] which compiled
> >> against 1.13.1in SQL Client, but it can not work in flink 1.14.0
> cluster,
> >> it failed due to the metric API compatibility broken.
> >>
> >> @Public
> >> public interface SourceReaderContext {
> >>
> >> MetricGroup metricGroup();
> >>
> >>
> >> @Public
> >> public interface SourceReaderContext {
> >>
> >> SourceReaderMetricGroup metricGroup();
> >>
> >>
> >> Shouldn't we mark it as @Deprecated and then delete it util 2.0.0
> version
> >> for @Public API as the our community rule [2] described? At least we
> should
> >> keep them across server minor versions (<major>.<minor>.<patch>).
> >>
> >> Although these changes can be tracked to voted FLIPs and it’s not the
> >> fault of a few developers, it show us the fact that we didn’t pay enough
> >> attention to back compatibility/forward compatibility.
> >>
> >> Best,
> >> Leonard
> >> [1]
> >>
> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
> >> [2]
> >> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> >>
> >>
>
>
Re: The Apache Flink should pay more attention to ensuring API
compatibility.
Posted by Chesnay Schepler <ch...@apache.org>.
We already have such tooling via japicmp; it's just that it is only
enabled for Public APIs.
You can probably generate a report via japicmp for all
PublicEvolging/Experimental APIs as well.
On 28/09/2021 15:17, Ingo Bürk wrote:
> Hi everyone,
>
> I think it would be best to support this process with tooling as much as
> possible, because humans are bound to make mistakes. FLINK-24138[1] should
> be a first step in this direction, but it wouldn't catch the cases
> discussed here.
> Maybe we should consider "capturing" the public API into some separate
> file(s) and validating against that in the CI such that structural changes
> to public APIs (moved classes, renamed / exchanged classes, changed
> signatures, …) can be caught as an error. That would raise awareness for
> any changes to public APIs and force a conscious decision.
>
> [1] https://issues.apache.org/jira/browse/FLINK-24138
>
>
> Best
> Ingo
>
> On Tue, Sep 28, 2021 at 2:49 PM Leonard Xu <xb...@gmail.com> wrote:
>
>>>> Not sure if this will happen in 1.15 already. We will needed automated
>>>> compatibility tests and a well-defined list of stable API.
>>> We are
>>> trying to provide forward compatibility: applications using `@Public`
>> APIs
>>> compiled against Flink 1.12.x, should work fine in Flink 1.13.x
>> Unfortunately, I also meet forward compatibility issue, when I do the
>> release 1.14 check, I try to use mysql-cdc connector[1] which compiled
>> against 1.13.1in SQL Client, but it can not work in flink 1.14.0 cluster,
>> it failed due to the metric API compatibility broken.
>>
>> @Public
>> public interface SourceReaderContext {
>>
>> MetricGroup metricGroup();
>>
>>
>> @Public
>> public interface SourceReaderContext {
>>
>> SourceReaderMetricGroup metricGroup();
>>
>>
>> Shouldn't we mark it as @Deprecated and then delete it util 2.0.0 version
>> for @Public API as the our community rule [2] described? At least we should
>> keep them across server minor versions (<major>.<minor>.<patch>).
>>
>> Although these changes can be tracked to voted FLIPs and it’s not the
>> fault of a few developers, it show us the fact that we didn’t pay enough
>> attention to back compatibility/forward compatibility.
>>
>> Best,
>> Leonard
>> [1]
>> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
>> [2]
>> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
>>
>>
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Ingo Bürk <in...@ververica.com>.
Hi everyone,
I think it would be best to support this process with tooling as much as
possible, because humans are bound to make mistakes. FLINK-24138[1] should
be a first step in this direction, but it wouldn't catch the cases
discussed here.
Maybe we should consider "capturing" the public API into some separate
file(s) and validating against that in the CI such that structural changes
to public APIs (moved classes, renamed / exchanged classes, changed
signatures, …) can be caught as an error. That would raise awareness for
any changes to public APIs and force a conscious decision.
[1] https://issues.apache.org/jira/browse/FLINK-24138
Best
Ingo
On Tue, Sep 28, 2021 at 2:49 PM Leonard Xu <xb...@gmail.com> wrote:
> >>
> >> Not sure if this will happen in 1.15 already. We will needed automated
> >> compatibility tests and a well-defined list of stable API.
>
> > We are
> > trying to provide forward compatibility: applications using `@Public`
> APIs
> > compiled against Flink 1.12.x, should work fine in Flink 1.13.x
>
> Unfortunately, I also meet forward compatibility issue, when I do the
> release 1.14 check, I try to use mysql-cdc connector[1] which compiled
> against 1.13.1in SQL Client, but it can not work in flink 1.14.0 cluster,
> it failed due to the metric API compatibility broken.
>
> @Public
> public interface SourceReaderContext {
>
> MetricGroup metricGroup();
>
>
> @Public
> public interface SourceReaderContext {
>
> SourceReaderMetricGroup metricGroup();
>
>
> Shouldn't we mark it as @Deprecated and then delete it util 2.0.0 version
> for @Public API as the our community rule [2] described? At least we should
> keep them across server minor versions (<major>.<minor>.<patch>).
>
> Although these changes can be tracked to voted FLIPs and it’s not the
> fault of a few developers, it show us the fact that we didn’t pay enough
> attention to back compatibility/forward compatibility.
>
> Best,
> Leonard
> [1]
> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
> [2]
> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
>
>
Re: 退订
Posted by Leonard Xu <xb...@gmail.com>.
Please send email to dev-unsubscribe@flink.apache.org if you want to unsubscribe the mail from dev@flink.apache.org, you can refer[1] for more details.
[1]https://flink.apache.org/zh/community.html#section
> 在 2021年10月8日，14:14，王卫光 <wa...@stevegame.cn> 写道：
>
> 退订，谢谢！
退订
Posted by 王卫光 <wa...@stevegame.cn>.
退订，谢谢！
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Jingsong Li <ji...@gmail.com>.
Hi all,
It was a wonderful discussion.
I think it can be made clear that PublicEvolving must not provide
cross version compatibility, which means that once the connector wants
to use new features, it must not be cross version compatible.
However, the connector must maintain multiple versions. Many users
only use the old version of Flink, but they need to use the new
connector. It's easy to upgrade a connector, but it's difficult to
upgrade the Flink version.
This seems to be unsolvable.
The hive's integration is a headache to support multiple Hive
versions. Arvid speaking of using reflection, Ah... HiveShim in
flink-connector-hive is doing this. However, reflection is ugly and
difficult to maintain.
When it is absolutely necessary to modify the API, can we consider
providing a mechanism similar to shim so that the connector can
directly use cross version Hacky... (maybe for just three versions)
(It doesn't have to be reflection mechanism. The current shim is not
good. But API changes always make API better. We do not restrict it.
Can we tolerate it outside?)
Best,
Jingsong
On Thu, Oct 7, 2021 at 9:00 PM Jark Wu <im...@gmail.com> wrote:
>
> Hi Arvid,
>
> Yes, reflection can solve this problem on the connector side, but this
> increases the burden to connectors.
> That also means all the existing released source connectors which use
> "metricGroup" will not work on 1.14,
> and they have to release a new version. It's just a problem of who changes
> the code and releases new versions.
>
> I agree with Piotr that we should
> "trying to support forward compatibility of `@PublicEvolving` APIs for one
> or two
> releases into the future might be a good rule of thumb."
>
> I think this is also the potential purpose that OpenInx started this
> discussion: the ecosystem needs
> a compatible API to build rich connectors.
>
>
> Best,
> Jark
>
> On Mon, 4 Oct 2021 at 16:22, Arvid Heise <ar...@ververica.com> wrote:
>
> > Hi Jark,
> >
> > I also don't see it as a blocker issue at all. If you want to access the
> > metric group across 1.13 and 1.14, you can use
> >
> > (MetricGroup) context.getClass().getMethod("metricGroup").invoke(context)
> >
> > But of course, you will not be able to access the new standardized metrics.
> > For that you will need to maintain two different source/binary builds,
> > since it's a new feature.
> >
> > I agree with Piotr, the issue is that we need a more standardized process
> > around PublicEvolving. Ideally, with every new minor release, we should
> > convert PublicEvolving to Public and Experimental to PublicEvolving. We
> > could extend the interfaces to capture a target version and a comment for
> > why the API is not Public yet. Before every release, we would go through
> > the annotated classes and either find a specific reason to keep the
> > annotation or move it towards Public. If we have a specific reason to keep
> > it Experimental/PublicEvolving, we should plan to address that reason with
> > the next release.
> >
> > We do have good checks in place for Public; we are just too slow with
> > ensuring that new API becomes Public.
> >
> > On Fri, Oct 1, 2021 at 9:41 PM Piotr Nowojski <pn...@apache.org>
> > wrote:
> >
> > > Hi,
> > >
> > > I don't understand why we are talking about this being a blocker issue?
> > New
> > > sources were not marked as @Public for a good reason until 1.14. I agree,
> > > we should try better at making APIs @Public sooner. I was even proposing
> > to
> > > create strict timeout rules (enforced via some automated checks) like
> > > (unless for a very very good reason) everything marked @PublicEvolving
> > > or @Experimental should be upgraded to @Public after for example 2 years
> > > [1]. But for example the new Sink API IMO is too fresh to make it
> > > `@Public`.
> > >
> > > It doesn't change the fact that if we could provide a compatibility layer
> > > between 1.13.x and 1.14.x for this SourceReaderContext issue, it would
> > be a
> > > nice thing to do. I would be -1 for keeping it forever, but trying to
> > > support forward compatibility of `@PublicEvolving` APIs for one or two
> > > releases into the future might be a good rule of thumb.
> > >
> > > Best, Piotrek
> > >
> > > [1] "[DISCUSS] Dealing with deprecated and legacy code in Flink" on the
> > dev
> > > mailing list
> > >
> > >
> > > pt., 1 paź 2021 o 16:56 Jark Wu <im...@gmail.com> napisał(a):
> > >
> > > > Hi Arvid,
> > > >
> > > > > Should we expect connector devs to release different connector
> > binaries
> > > > for different Flink minors?
> > > > From the discussion of this thread, I think the answer is obviously
> > > "not",
> > > > otherwise OpenInx won't start
> > > > this discussion. As a maintainer of flink-cdc-connector, I have to say
> > > > that it's very hard to release
> > > > connectors for different flink versions. Usually, the connector
> > > community
> > > > doesn't have so much time to
> > > > maintain different branches/modules/code for different flink versions.
> > > >
> > > > > If we change it back, then a specific connector would work for 1.14.1
> > > and
> > > > 1.13.X but not for 1.14.0 and this would be even more confusing.
> > > > I think this is fine. IMO, this is a blocker issue of 1.14.0 which
> > breaks
> > > > Source connectors.
> > > > We should suggest users to use 1.14.1 if they use Source connectors.
> > > >
> > > > Best,
> > > > Jark
> > > >
> > > >
> > > > On Fri, 1 Oct 2021 at 19:05, Arvid Heise <ar...@ververica.com> wrote:
> > > >
> > > > > The issue is that if we do not mark them as Public, we will always
> > have
> > > > > incompatibilities. The change of SourceReaderContext#metricGroup is
> > > > > perfectly fine according to the annotation. The implications that we
> > > see
> > > > > here just mean that the interfaces have been expected to be Public.
> > > > >
> > > > > And now the question is what do we expect?
> > > > > Should we expect connector devs to release different connector
> > binaries
> > > > > for different Flink minors? Then PublicEvolving is fine.
> > > > > If we expect that the same connector can work across multiple Flink
> > > > > versions, we need to go into Public.
> > > > >
> > > > > It doesn't make sense to keep them PublicEvolving on the annotation
> > but
> > > > > implicitly assume them to be Public.
> > > > >
> > > > > @Jark Wu <im...@gmail.com> I don't see a way to revert the change
> > of
> > > > > SourceReaderContext#metricGroup. For now, connector devs that expose
> > > > > metrics need to release 2 versions. If we change it back, then a
> > > specific
> > > > > connector would work for 1.14.1 and 1.13.X but not for 1.14.0 and
> > this
> > > > > would be even more confusing.
> > > > >
> > > > > On Fri, Oct 1, 2021 at 10:49 AM Ingo Bürk <in...@ververica.com>
> > wrote:
> > > > >
> > > > >> Hi,
> > > > >>
> > > > >> > [...] but also the new Source/Sink APIs as public
> > > > >>
> > > > >> I'm not really involved in the new Source/Sink APIs and will happily
> > > > >> listen to the developers working with them here, but since they are
> > > > new, we
> > > > >> should also be careful not to mark them as stable too quickly. We've
> > > > only
> > > > >> begun updating the existing connectors to these interfaces at the
> > > > moment.
> > > > >> Making more progress here and keeping new APIs as Evolving for a
> > > couple
> > > > of
> > > > >> minor releases is probably still a good idea. Maybe we should even
> > > have
> > > > >> actual rules on when APIs can/should be promoted?
> > > > >>
> > > > >> More actively checking backwards-compatibility during a release
> > sounds
> > > > >> like a great idea regardless, of course.
> > > > >>
> > > > >>
> > > > >> Ingo
> > > > >>
> > > > >> On Fri, Oct 1, 2021 at 9:19 AM Jark Wu <im...@gmail.com> wrote:
> > > > >>
> > > > >>> Hi all,
> > > > >>>
> > > > >>> Nice thread and great discussion! Ecosystem is one of the most
> > > > important
> > > > >>> things
> > > > >>> to the Flink community, we should pay more attention to API
> > > > >>> compatibility.
> > > > >>>
> > > > >>> Marking all connector APIs @Public is a good idea, not only mark
> > the
> > > > >>> Table/SQL
> > > > >>> connector APIs public, but also the new Source/Sink APIs as public.
> > > > >>> Besides, we should also add a check item to the Verify Release
> > > > >>> documentation[1]
> > > > >>> to verify the release is backward-compatible for connectors. From
> > my
> > > > >>> point
> > > > >>> of view,
> > > > >>> such backward incompatibility should cancel the vote.
> > > > >>>
> > > > >>> Regarding the SourceReaderContext#metricGroup compatibility problem
> > > in
> > > > >>> 1.14.0, I would
> > > > >>> suggest starting a new discussion thread to see whether we have any
> > > > idea
> > > > >>> to
> > > > >>> fix it. We should
> > > > >>> fix it ASAP! Otherwise iceberg/hudi/cdc communities will get
> > > frustrated
> > > > >>> again when upgrading
> > > > >>> to 1.14. Maybe we still have time to release a minor version,
> > > because
> > > > >>> there is no
> > > > >>> connector upgraded to 1.14.0 yet. What do you think? @Leonard Xu
> > > > >>> <xb...@gmail.com> @Arvid Heise <ar...@ververica.com> @Piotr
> > > Nowojski
> > > > >>> <pi...@ververica.com>
> > > > >>>
> > > > >>> Best,
> > > > >>> Jark
> > > > >>>
> > > > >>> [1]:
> > > > >>>
> > > > >>>
> > > >
> > >
> > https://cwiki.apache.org/confluence/display/FLINK/Verifying+a+Flink+Release
> > > > >>>
> > > > >>> On Wed, 29 Sept 2021 at 09:46, OpenInx <op...@gmail.com> wrote:
> > > > >>>
> > > > >>> > > Apart from this being `@PublicEvolving`
> > > > >>> >
> > > > >>> > From my perspective, annotating the 'DynamicTableSink' to be a
> > > > >>> > 'PublicEvolving' class is not reasonable, because that means devs
> > > > could
> > > > >>> > just change the basic API which all downstream connectors are
> > > > >>> depending on
> > > > >>> > easily when iterating flink from 1.12 to 1.13 (according to the
> > > wiki
> > > > >>> [1]).
> > > > >>> > This implies all downstream maintainers must take on this
> > > maintenance
> > > > >>> > burden, and it also makes our flink ecosystem very fragile.
> > > > Changing
> > > > >>> the
> > > > >>> > 'DynamicTableSink' between two major versions sounds reasonable
> > to
> > > > me,
> > > > >>> but
> > > > >>> > unreasonable for uncompatibility changes between two minor
> > > versions.
> > > > >>> I
> > > > >>> > think we may need to check those API which are annotated
> > > > >>> 'PublicEnvoling'
> > > > >>> > while should be 'Public' because of the dependency from all
> > > > >>> connectors.
> > > > >>> > We should ensure the stability of those APIs that are necessary
> > to
> > > > >>> > implement the connector, and at the same time implement the
> > updated
> > > > v2
> > > > >>> > version of the API. After all v2 APIs are considered stable, we
> > > will
> > > > >>> mark
> > > > >>> > them as stable. Instead of releasing a version of the API, some
> > of
> > > > the
> > > > >>> APIs
> > > > >>> > necessary to implement the connector are marked as stable and
> > some
> > > > are
> > > > >>> > marked as unstable, which is very unfriendly to downstream.
> > Because
> > > > >>> > downstream essentially every upgrade requires refactoring of the
> > > > code.
> > > > >>> >
> > > > >>> > > We are trying to provide forward compatibility: applications
> > > using
> > > > >>> > `@Public` APIs compiled against Flink 1.12.x, should work fine in
> > > > Flink
> > > > >>> > 1.13.x
> > > > >>> >
> > > > >>> > Thanks for clarifying this. Sounds reasonable to me, then we
> > > apache
> > > > >>> > iceberg could just use flink 1.12.x to build the flink+iceberg
> > > > >>> connector
> > > > >>> > and should make all the tests work fine for both flink 1.12 &
> > flink
> > > > >>> 1.13.
> > > > >>> > For the `ResolvedCatalogTable` changes, I don't think it
> > > guarantees
> > > > >>> the
> > > > >>> > forward compatibility as you said, because the
> > > > >>> flink-iceberg-runtime.jar
> > > > >>> > compiled by flink 1.12 can still not works fine in flink 1.13
> > > because
> > > > >>> it
> > > > >>> > will need the flink1.12's `CatalogTable` to be cast to a
> > flink1.13
> > > > 's
> > > > >>> > `ResolvedCatalogTable`. In general, I agree that forward
> > > > >>> compatibility is
> > > > >>> > a more clear compatibility guarantee.
> > > > >>> >
> > > > >>> > [1].
> > > > >>> >
> > > > >>>
> > > >
> > https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> > > > >>> >
> > > > >>> >
> > > > >>> > On Tue, Sep 28, 2021 at 10:33 PM David Morávek <dm...@apache.org>
> > > > >>> wrote:
> > > > >>> >
> > > > >>> > > >
> > > > >>> > > > I think we have a compile time checks for breaking changes in
> > > > >>> `@Public`
> > > > >>> > > > marked classes/interfaces using japicmp [1].
> > > > >>> > >
> > > > >>> > >
> > > > >>> > > Nice, thanks for pointing that out, I'll take a closer look at
> > it
> > > > ;)
> > > > >>> > >
> > > > >>> > > Best,
> > > > >>> > > D.
> > > > >>> > >
> > > > >>> > > On Tue, Sep 28, 2021 at 4:14 PM Piotr Nowojski <
> > > > pnowojski@apache.org
> > > > >>> >
> > > > >>> > > wrote:
> > > > >>> > >
> > > > >>> > > > > - We don't have any safeguards for stable API breaks. Big
> > +1
> > > > for
> > > > >>> > Ingo's
> > > > >>> > > > effort with architectural tests [3].
> > > > >>> > > >
> > > > >>> > > > I think we have a compile time checks for breaking changes in
> > > > >>> `@Public`
> > > > >>> > > > marked classes/interfaces using japicmp [1].
> > > > >>> > > >
> > > > >>> > > > Piotrek
> > > > >>> > > >
> > > > >>> > > > [1]
> > > > >>> https://github.com/apache/flink/blob/master/pom.xml#L2014:L2084
> > > > >>> > > >
> > > > >>> > > > wt., 28 wrz 2021 o 15:59 David Morávek <dm...@apache.org>
> > > > >>> napisał(a):
> > > > >>> > > >
> > > > >>> > > > > This is a super interesting topic and there is already a
> > > great
> > > > >>> > > > discussion.
> > > > >>> > > > > Here are few thoughts:
> > > > >>> > > > >
> > > > >>> > > > > - There is a delicate balance between fast delivery of the
> > > new
> > > > >>> > features
> > > > >>> > > > and
> > > > >>> > > > > API stability. Even though we should be careful with
> > breaking
> > > > >>> > evolving
> > > > >>> > > > > interfaces, it shouldn't stop us from making fast progress
> > /
> > > > >>> iterate
> > > > >>> > on
> > > > >>> > > > > features.
> > > > >>> > > > > - There are two camps of users. One camp prefers more
> > > frequent
> > > > >>> > > releases /
> > > > >>> > > > > new features (early adopters) and second that prefer less
> > > > >>> frequent
> > > > >>> > > stable
> > > > >>> > > > > releases. There was already a great discussion about this
> > at
> > > > >>> Flink
> > > > >>> > 1.14
> > > > >>> > > > > thread [1].
> > > > >>> > > > > - We're already trying to be explicit about which API's may
> > > > >>> break via
> > > > >>> > > > > annotations and the feature radar [2]. Stability
> > annotations
> > > > are
> > > > >>> a
> > > > >>> > well
> > > > >>> > > > > known concept used by many projects. I think we still can
> > go
> > > > bit
> > > > >>> > > further
> > > > >>> > > > > here and aim for an IDE support (for example usages of
> > guava
> > > > >>> > > > @Experimental
> > > > >>> > > > > interfaces get highlighted, raising more awareness about
> > > > >>> potential
> > > > >>> > > > issues).
> > > > >>> > > > > I'm not sure how this IDE integration works though.
> > > > >>> > > > > - We don't have any safeguards for stable API breaks. Big
> > +1
> > > > for
> > > > >>> > Ingo's
> > > > >>> > > > > effort with architectural tests [3].
> > > > >>> > > > >
> > > > >>> > > > > [1]
> > > > >>> > > > >
> > > > >>> > > > >
> > > > >>> > > >
> > > > >>> > >
> > > > >>> >
> > > > >>>
> > > >
> > >
> > https://lists.apache.org/thread.html/r76e1cdba577c6f4d6c86b23fdaeb53c4e3744c20d0b3e850fc2e14a7%40%3Cdev.flink.apache.org%3E
> > > > >>> > > > > [2] https://flink.apache.org/roadmap.html
> > > > >>> > > > > [3] https://issues.apache.org/jira/browse/FLINK-24138
> > > > >>> > > > >
> > > > >>> > > > > Best,
> > > > >>> > > > > D.
> > > > >>> > > > >
> > > > >>> > > > > On Tue, Sep 28, 2021 at 3:49 PM Leonard Xu <
> > > xbjtdcq@gmail.com>
> > > > >>> > wrote:
> > > > >>> > > > >
> > > > >>> > > > > > Thanks Piotr for the kindly reply, what confused me is
> > that
> > > > >>> > > > > > `SourceReaderContext` was marked @Public when it was born
> > > in
> > > > >>> flink
> > > > >>> > > > 1.11,
> > > > >>> > > > > > and then it was corrected to @PublicEvolving in 1.11 -_-,
> > > and
> > > > >>> > finally
> > > > >>> > > > it
> > > > >>> > > > > > was changed to @Public again...
> > > > >>> > > > > >
> > > > >>> > > > > > As Flink and Flink ecosystem(Flink CDC connectors)
> > > > developer, I
> > > > >>> > think
> > > > >>> > > > > what
> > > > >>> > > > > > we're discussing is meaningful and I’d like to help
> > improve
> > > > >>> those
> > > > >>> > > > Public
> > > > >>> > > > > > API check for those changes.
> > > > >>> > > > > > Chesnay’s tips is a good idea that maybe we can use the
> > > tool
> > > > >>> like
> > > > >>> > > > japicmp
> > > > >>> > > > > > to do the check for every PR in CI phase.
> > > > >>> > > > > >
> > > > >>> > > > > > Best,
> > > > >>> > > > > > Leonard
> > > > >>> > > > > >
> > > > >>> > > > > >
> > > > >>> > > > > > > 在 2021年9月28日，21:15，Piotr Nowojski <
> > pnowojski@apache.org>
> > > > 写道：
> > > > >>> > > > > > >
> > > > >>> > > > > > > Hi Leonard,
> > > > >>> > > > > > >
> > > > >>> > > > > > > Sorry for this causing you troubles, however that
> > change
> > > in
> > > > >>> the
> > > > >>> > > > return
> > > > >>> > > > > > type
> > > > >>> > > > > > > was done while this class still has been marked as
> > > > >>> > > > > `@PublicEvolving`[1].
> > > > >>> > > > > > As
> > > > >>> > > > > > > of 1.13.x `SourceReaderContext` was `@PublicEvolving`
> > and
> > > > it
> > > > >>> was
> > > > >>> > > > marked
> > > > >>> > > > > > as
> > > > >>> > > > > > > `@Public` only starting from Flink 1.14.0 [2]. Probably
> > > > what
> > > > >>> > > confused
> > > > >>> > > > > you
> > > > >>> > > > > > > was that both of those changes (changing the return
> > type
> > > > and
> > > > >>> > making
> > > > >>> > > > it
> > > > >>> > > > > > > `@Public`) happened in the same release.
> > > > >>> > > > > > >
> > > > >>> > > > > > > However, those changes (`SourceReaderContext` and
> > > > >>> > > > > `ResolvedCatalogTable`)
> > > > >>> > > > > > > should have been clearly mentioned in the release notes
> > > > with
> > > > >>> an
> > > > >>> > > > upgrade
> > > > >>> > > > > > > guide.
> > > > >>> > > > > > >
> > > > >>> > > > > > > Best, Piotrek
> > > > >>> > > > > > >
> > > > >>> > > > > > > [1]
> > > > >>> > > > > > >
> > > > >>> > > > > >
> > > > >>> > > > >
> > > > >>> > > >
> > > > >>> > >
> > > > >>> >
> > > > >>>
> > > >
> > >
> > https://github.com/apache/flink/commit/7f3636f6b4f8bac415a7db85917ad849636bd730#diff-a247a24ccd1afc07c5d690a8a58b1f6584329925fdf0d7dc89361b90d621b7f2R31
> > > > >>> > > > > > > [2] https://issues.apache.org/jira/browse/FLINK-22357
> > > > >>> > > > > > >
> > > > >>> > > > > > > wt., 28 wrz 2021 o 14:49 Leonard Xu <xbjtdcq@gmail.com
> > >
> > > > >>> > > napisał(a):
> > > > >>> > > > > > >
> > > > >>> > > > > > >>>>
> > > > >>> > > > > > >>>> Not sure if this will happen in 1.15 already. We
> > will
> > > > >>> needed
> > > > >>> > > > > automated
> > > > >>> > > > > > >>>> compatibility tests and a well-defined list of
> > stable
> > > > API.
> > > > >>> > > > > > >>
> > > > >>> > > > > > >>> We are
> > > > >>> > > > > > >>> trying to provide forward compatibility: applications
> > > > using
> > > > >>> > > > `@Public`
> > > > >>> > > > > > >> APIs
> > > > >>> > > > > > >>> compiled against Flink 1.12.x, should work fine in
> > > Flink
> > > > >>> 1.13.x
> > > > >>> > > > > > >>
> > > > >>> > > > > > >> Unfortunately, I also meet forward compatibility
> > issue,
> > > > >>> when I
> > > > >>> > do
> > > > >>> > > > the
> > > > >>> > > > > > >> release 1.14 check, I try to use mysql-cdc
> > connector[1]
> > > > >>> which
> > > > >>> > > > compiled
> > > > >>> > > > > > >> against 1.13.1in SQL Client, but it can not work in
> > > flink
> > > > >>> 1.14.0
> > > > >>> > > > > > cluster,
> > > > >>> > > > > > >> it failed due to the metric API compatibility broken.
> > > > >>> > > > > > >>
> > > > >>> > > > > > >> @Public
> > > > >>> > > > > > >> public interface SourceReaderContext {
> > > > >>> > > > > > >>
> > > > >>> > > > > > >> MetricGroup metricGroup();
> > > > >>> > > > > > >>
> > > > >>> > > > > > >>
> > > > >>> > > > > > >> @Public
> > > > >>> > > > > > >> public interface SourceReaderContext {
> > > > >>> > > > > > >>
> > > > >>> > > > > > >> SourceReaderMetricGroup metricGroup();
> > > > >>> > > > > > >>
> > > > >>> > > > > > >>
> > > > >>> > > > > > >> Shouldn't we mark it as @Deprecated and then delete it
> > > > util
> > > > >>> > 2.0.0
> > > > >>> > > > > > version
> > > > >>> > > > > > >> for @Public API as the our community rule [2]
> > described?
> > > > At
> > > > >>> > least
> > > > >>> > > we
> > > > >>> > > > > > should
> > > > >>> > > > > > >> keep them across server minor versions
> > > > >>> > (<major>.<minor>.<patch>).
> > > > >>> > > > > > >>
> > > > >>> > > > > > >> Although these changes can be tracked to voted FLIPs
> > and
> > > > >>> it’s
> > > > >>> > not
> > > > >>> > > > the
> > > > >>> > > > > > >> fault of a few developers, it show us the fact that we
> > > > >>> didn’t
> > > > >>> > pay
> > > > >>> > > > > enough
> > > > >>> > > > > > >> attention to back compatibility/forward compatibility.
> > > > >>> > > > > > >>
> > > > >>> > > > > > >> Best,
> > > > >>> > > > > > >> Leonard
> > > > >>> > > > > > >> [1]
> > > > >>> > > > > > >>
> > > > >>> > > > > >
> > > > >>> > > > >
> > > > >>> > > >
> > > > >>> > >
> > > > >>> >
> > > > >>>
> > > >
> > >
> > https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
> > > > >>> > > > > > >> [2]
> > > > >>> > > > > > >>
> > > > >>> > > > >
> > > > >>> > >
> > > > >>>
> > > >
> > https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> > > > >>> > > > > > >>
> > > > >>> > > > > > >>
> > > > >>> > > > > >
> > > > >>> > > > > >
> > > > >>> > > > >
> > > > >>> > > >
> > > > >>> > >
> > > > >>> >
> > > > >>>
> > > > >>
> > > >
> > >
> >
--
Best, Jingsong Lee
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Jark Wu <im...@gmail.com>.
Hi Arvid,
Yes, reflection can solve this problem on the connector side, but this
increases the burden to connectors.
That also means all the existing released source connectors which use
"metricGroup" will not work on 1.14,
and they have to release a new version. It's just a problem of who changes
the code and releases new versions.
I agree with Piotr that we should
"trying to support forward compatibility of `@PublicEvolving` APIs for one
or two
releases into the future might be a good rule of thumb."
I think this is also the potential purpose that OpenInx started this
discussion: the ecosystem needs
a compatible API to build rich connectors.
Best,
Jark
On Mon, 4 Oct 2021 at 16:22, Arvid Heise <ar...@ververica.com> wrote:
> Hi Jark,
>
> I also don't see it as a blocker issue at all. If you want to access the
> metric group across 1.13 and 1.14, you can use
>
> (MetricGroup) context.getClass().getMethod("metricGroup").invoke(context)
>
> But of course, you will not be able to access the new standardized metrics.
> For that you will need to maintain two different source/binary builds,
> since it's a new feature.
>
> I agree with Piotr, the issue is that we need a more standardized process
> around PublicEvolving. Ideally, with every new minor release, we should
> convert PublicEvolving to Public and Experimental to PublicEvolving. We
> could extend the interfaces to capture a target version and a comment for
> why the API is not Public yet. Before every release, we would go through
> the annotated classes and either find a specific reason to keep the
> annotation or move it towards Public. If we have a specific reason to keep
> it Experimental/PublicEvolving, we should plan to address that reason with
> the next release.
>
> We do have good checks in place for Public; we are just too slow with
> ensuring that new API becomes Public.
>
> On Fri, Oct 1, 2021 at 9:41 PM Piotr Nowojski <pn...@apache.org>
> wrote:
>
> > Hi,
> >
> > I don't understand why we are talking about this being a blocker issue?
> New
> > sources were not marked as @Public for a good reason until 1.14. I agree,
> > we should try better at making APIs @Public sooner. I was even proposing
> to
> > create strict timeout rules (enforced via some automated checks) like
> > (unless for a very very good reason) everything marked @PublicEvolving
> > or @Experimental should be upgraded to @Public after for example 2 years
> > [1]. But for example the new Sink API IMO is too fresh to make it
> > `@Public`.
> >
> > It doesn't change the fact that if we could provide a compatibility layer
> > between 1.13.x and 1.14.x for this SourceReaderContext issue, it would
> be a
> > nice thing to do. I would be -1 for keeping it forever, but trying to
> > support forward compatibility of `@PublicEvolving` APIs for one or two
> > releases into the future might be a good rule of thumb.
> >
> > Best, Piotrek
> >
> > [1] "[DISCUSS] Dealing with deprecated and legacy code in Flink" on the
> dev
> > mailing list
> >
> >
> > pt., 1 paź 2021 o 16:56 Jark Wu <im...@gmail.com> napisał(a):
> >
> > > Hi Arvid,
> > >
> > > > Should we expect connector devs to release different connector
> binaries
> > > for different Flink minors?
> > > From the discussion of this thread, I think the answer is obviously
> > "not",
> > > otherwise OpenInx won't start
> > > this discussion. As a maintainer of flink-cdc-connector, I have to say
> > > that it's very hard to release
> > > connectors for different flink versions. Usually, the connector
> > community
> > > doesn't have so much time to
> > > maintain different branches/modules/code for different flink versions.
> > >
> > > > If we change it back, then a specific connector would work for 1.14.1
> > and
> > > 1.13.X but not for 1.14.0 and this would be even more confusing.
> > > I think this is fine. IMO, this is a blocker issue of 1.14.0 which
> breaks
> > > Source connectors.
> > > We should suggest users to use 1.14.1 if they use Source connectors.
> > >
> > > Best,
> > > Jark
> > >
> > >
> > > On Fri, 1 Oct 2021 at 19:05, Arvid Heise <ar...@ververica.com> wrote:
> > >
> > > > The issue is that if we do not mark them as Public, we will always
> have
> > > > incompatibilities. The change of SourceReaderContext#metricGroup is
> > > > perfectly fine according to the annotation. The implications that we
> > see
> > > > here just mean that the interfaces have been expected to be Public.
> > > >
> > > > And now the question is what do we expect?
> > > > Should we expect connector devs to release different connector
> binaries
> > > > for different Flink minors? Then PublicEvolving is fine.
> > > > If we expect that the same connector can work across multiple Flink
> > > > versions, we need to go into Public.
> > > >
> > > > It doesn't make sense to keep them PublicEvolving on the annotation
> but
> > > > implicitly assume them to be Public.
> > > >
> > > > @Jark Wu <im...@gmail.com> I don't see a way to revert the change
> of
> > > > SourceReaderContext#metricGroup. For now, connector devs that expose
> > > > metrics need to release 2 versions. If we change it back, then a
> > specific
> > > > connector would work for 1.14.1 and 1.13.X but not for 1.14.0 and
> this
> > > > would be even more confusing.
> > > >
> > > > On Fri, Oct 1, 2021 at 10:49 AM Ingo Bürk <in...@ververica.com>
> wrote:
> > > >
> > > >> Hi,
> > > >>
> > > >> > [...] but also the new Source/Sink APIs as public
> > > >>
> > > >> I'm not really involved in the new Source/Sink APIs and will happily
> > > >> listen to the developers working with them here, but since they are
> > > new, we
> > > >> should also be careful not to mark them as stable too quickly. We've
> > > only
> > > >> begun updating the existing connectors to these interfaces at the
> > > moment.
> > > >> Making more progress here and keeping new APIs as Evolving for a
> > couple
> > > of
> > > >> minor releases is probably still a good idea. Maybe we should even
> > have
> > > >> actual rules on when APIs can/should be promoted?
> > > >>
> > > >> More actively checking backwards-compatibility during a release
> sounds
> > > >> like a great idea regardless, of course.
> > > >>
> > > >>
> > > >> Ingo
> > > >>
> > > >> On Fri, Oct 1, 2021 at 9:19 AM Jark Wu <im...@gmail.com> wrote:
> > > >>
> > > >>> Hi all,
> > > >>>
> > > >>> Nice thread and great discussion! Ecosystem is one of the most
> > > important
> > > >>> things
> > > >>> to the Flink community, we should pay more attention to API
> > > >>> compatibility.
> > > >>>
> > > >>> Marking all connector APIs @Public is a good idea, not only mark
> the
> > > >>> Table/SQL
> > > >>> connector APIs public, but also the new Source/Sink APIs as public.
> > > >>> Besides, we should also add a check item to the Verify Release
> > > >>> documentation[1]
> > > >>> to verify the release is backward-compatible for connectors. From
> my
> > > >>> point
> > > >>> of view,
> > > >>> such backward incompatibility should cancel the vote.
> > > >>>
> > > >>> Regarding the SourceReaderContext#metricGroup compatibility problem
> > in
> > > >>> 1.14.0, I would
> > > >>> suggest starting a new discussion thread to see whether we have any
> > > idea
> > > >>> to
> > > >>> fix it. We should
> > > >>> fix it ASAP! Otherwise iceberg/hudi/cdc communities will get
> > frustrated
> > > >>> again when upgrading
> > > >>> to 1.14. Maybe we still have time to release a minor version,
> > because
> > > >>> there is no
> > > >>> connector upgraded to 1.14.0 yet. What do you think? @Leonard Xu
> > > >>> <xb...@gmail.com> @Arvid Heise <ar...@ververica.com> @Piotr
> > Nowojski
> > > >>> <pi...@ververica.com>
> > > >>>
> > > >>> Best,
> > > >>> Jark
> > > >>>
> > > >>> [1]:
> > > >>>
> > > >>>
> > >
> >
> https://cwiki.apache.org/confluence/display/FLINK/Verifying+a+Flink+Release
> > > >>>
> > > >>> On Wed, 29 Sept 2021 at 09:46, OpenInx <op...@gmail.com> wrote:
> > > >>>
> > > >>> > > Apart from this being `@PublicEvolving`
> > > >>> >
> > > >>> > From my perspective, annotating the 'DynamicTableSink' to be a
> > > >>> > 'PublicEvolving' class is not reasonable, because that means devs
> > > could
> > > >>> > just change the basic API which all downstream connectors are
> > > >>> depending on
> > > >>> > easily when iterating flink from 1.12 to 1.13 (according to the
> > wiki
> > > >>> [1]).
> > > >>> > This implies all downstream maintainers must take on this
> > maintenance
> > > >>> > burden, and it also makes our flink ecosystem very fragile.
> > > Changing
> > > >>> the
> > > >>> > 'DynamicTableSink' between two major versions sounds reasonable
> to
> > > me,
> > > >>> but
> > > >>> > unreasonable for uncompatibility changes between two minor
> > versions.
> > > >>> I
> > > >>> > think we may need to check those API which are annotated
> > > >>> 'PublicEnvoling'
> > > >>> > while should be 'Public' because of the dependency from all
> > > >>> connectors.
> > > >>> > We should ensure the stability of those APIs that are necessary
> to
> > > >>> > implement the connector, and at the same time implement the
> updated
> > > v2
> > > >>> > version of the API. After all v2 APIs are considered stable, we
> > will
> > > >>> mark
> > > >>> > them as stable. Instead of releasing a version of the API, some
> of
> > > the
> > > >>> APIs
> > > >>> > necessary to implement the connector are marked as stable and
> some
> > > are
> > > >>> > marked as unstable, which is very unfriendly to downstream.
> Because
> > > >>> > downstream essentially every upgrade requires refactoring of the
> > > code.
> > > >>> >
> > > >>> > > We are trying to provide forward compatibility: applications
> > using
> > > >>> > `@Public` APIs compiled against Flink 1.12.x, should work fine in
> > > Flink
> > > >>> > 1.13.x
> > > >>> >
> > > >>> > Thanks for clarifying this. Sounds reasonable to me, then we
> > apache
> > > >>> > iceberg could just use flink 1.12.x to build the flink+iceberg
> > > >>> connector
> > > >>> > and should make all the tests work fine for both flink 1.12 &
> flink
> > > >>> 1.13.
> > > >>> > For the `ResolvedCatalogTable` changes, I don't think it
> > guarantees
> > > >>> the
> > > >>> > forward compatibility as you said, because the
> > > >>> flink-iceberg-runtime.jar
> > > >>> > compiled by flink 1.12 can still not works fine in flink 1.13
> > because
> > > >>> it
> > > >>> > will need the flink1.12's `CatalogTable` to be cast to a
> flink1.13
> > > 's
> > > >>> > `ResolvedCatalogTable`. In general, I agree that forward
> > > >>> compatibility is
> > > >>> > a more clear compatibility guarantee.
> > > >>> >
> > > >>> > [1].
> > > >>> >
> > > >>>
> > >
> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> > > >>> >
> > > >>> >
> > > >>> > On Tue, Sep 28, 2021 at 10:33 PM David Morávek <dm...@apache.org>
> > > >>> wrote:
> > > >>> >
> > > >>> > > >
> > > >>> > > > I think we have a compile time checks for breaking changes in
> > > >>> `@Public`
> > > >>> > > > marked classes/interfaces using japicmp [1].
> > > >>> > >
> > > >>> > >
> > > >>> > > Nice, thanks for pointing that out, I'll take a closer look at
> it
> > > ;)
> > > >>> > >
> > > >>> > > Best,
> > > >>> > > D.
> > > >>> > >
> > > >>> > > On Tue, Sep 28, 2021 at 4:14 PM Piotr Nowojski <
> > > pnowojski@apache.org
> > > >>> >
> > > >>> > > wrote:
> > > >>> > >
> > > >>> > > > > - We don't have any safeguards for stable API breaks. Big
> +1
> > > for
> > > >>> > Ingo's
> > > >>> > > > effort with architectural tests [3].
> > > >>> > > >
> > > >>> > > > I think we have a compile time checks for breaking changes in
> > > >>> `@Public`
> > > >>> > > > marked classes/interfaces using japicmp [1].
> > > >>> > > >
> > > >>> > > > Piotrek
> > > >>> > > >
> > > >>> > > > [1]
> > > >>> https://github.com/apache/flink/blob/master/pom.xml#L2014:L2084
> > > >>> > > >
> > > >>> > > > wt., 28 wrz 2021 o 15:59 David Morávek <dm...@apache.org>
> > > >>> napisał(a):
> > > >>> > > >
> > > >>> > > > > This is a super interesting topic and there is already a
> > great
> > > >>> > > > discussion.
> > > >>> > > > > Here are few thoughts:
> > > >>> > > > >
> > > >>> > > > > - There is a delicate balance between fast delivery of the
> > new
> > > >>> > features
> > > >>> > > > and
> > > >>> > > > > API stability. Even though we should be careful with
> breaking
> > > >>> > evolving
> > > >>> > > > > interfaces, it shouldn't stop us from making fast progress
> /
> > > >>> iterate
> > > >>> > on
> > > >>> > > > > features.
> > > >>> > > > > - There are two camps of users. One camp prefers more
> > frequent
> > > >>> > > releases /
> > > >>> > > > > new features (early adopters) and second that prefer less
> > > >>> frequent
> > > >>> > > stable
> > > >>> > > > > releases. There was already a great discussion about this
> at
> > > >>> Flink
> > > >>> > 1.14
> > > >>> > > > > thread [1].
> > > >>> > > > > - We're already trying to be explicit about which API's may
> > > >>> break via
> > > >>> > > > > annotations and the feature radar [2]. Stability
> annotations
> > > are
> > > >>> a
> > > >>> > well
> > > >>> > > > > known concept used by many projects. I think we still can
> go
> > > bit
> > > >>> > > further
> > > >>> > > > > here and aim for an IDE support (for example usages of
> guava
> > > >>> > > > @Experimental
> > > >>> > > > > interfaces get highlighted, raising more awareness about
> > > >>> potential
> > > >>> > > > issues).
> > > >>> > > > > I'm not sure how this IDE integration works though.
> > > >>> > > > > - We don't have any safeguards for stable API breaks. Big
> +1
> > > for
> > > >>> > Ingo's
> > > >>> > > > > effort with architectural tests [3].
> > > >>> > > > >
> > > >>> > > > > [1]
> > > >>> > > > >
> > > >>> > > > >
> > > >>> > > >
> > > >>> > >
> > > >>> >
> > > >>>
> > >
> >
> https://lists.apache.org/thread.html/r76e1cdba577c6f4d6c86b23fdaeb53c4e3744c20d0b3e850fc2e14a7%40%3Cdev.flink.apache.org%3E
> > > >>> > > > > [2] https://flink.apache.org/roadmap.html
> > > >>> > > > > [3] https://issues.apache.org/jira/browse/FLINK-24138
> > > >>> > > > >
> > > >>> > > > > Best,
> > > >>> > > > > D.
> > > >>> > > > >
> > > >>> > > > > On Tue, Sep 28, 2021 at 3:49 PM Leonard Xu <
> > xbjtdcq@gmail.com>
> > > >>> > wrote:
> > > >>> > > > >
> > > >>> > > > > > Thanks Piotr for the kindly reply, what confused me is
> that
> > > >>> > > > > > `SourceReaderContext` was marked @Public when it was born
> > in
> > > >>> flink
> > > >>> > > > 1.11,
> > > >>> > > > > > and then it was corrected to @PublicEvolving in 1.11 -_-,
> > and
> > > >>> > finally
> > > >>> > > > it
> > > >>> > > > > > was changed to @Public again...
> > > >>> > > > > >
> > > >>> > > > > > As Flink and Flink ecosystem(Flink CDC connectors)
> > > developer, I
> > > >>> > think
> > > >>> > > > > what
> > > >>> > > > > > we're discussing is meaningful and I’d like to help
> improve
> > > >>> those
> > > >>> > > > Public
> > > >>> > > > > > API check for those changes.
> > > >>> > > > > > Chesnay’s tips is a good idea that maybe we can use the
> > tool
> > > >>> like
> > > >>> > > > japicmp
> > > >>> > > > > > to do the check for every PR in CI phase.
> > > >>> > > > > >
> > > >>> > > > > > Best,
> > > >>> > > > > > Leonard
> > > >>> > > > > >
> > > >>> > > > > >
> > > >>> > > > > > > 在 2021年9月28日，21:15，Piotr Nowojski <
> pnowojski@apache.org>
> > > 写道：
> > > >>> > > > > > >
> > > >>> > > > > > > Hi Leonard,
> > > >>> > > > > > >
> > > >>> > > > > > > Sorry for this causing you troubles, however that
> change
> > in
> > > >>> the
> > > >>> > > > return
> > > >>> > > > > > type
> > > >>> > > > > > > was done while this class still has been marked as
> > > >>> > > > > `@PublicEvolving`[1].
> > > >>> > > > > > As
> > > >>> > > > > > > of 1.13.x `SourceReaderContext` was `@PublicEvolving`
> and
> > > it
> > > >>> was
> > > >>> > > > marked
> > > >>> > > > > > as
> > > >>> > > > > > > `@Public` only starting from Flink 1.14.0 [2]. Probably
> > > what
> > > >>> > > confused
> > > >>> > > > > you
> > > >>> > > > > > > was that both of those changes (changing the return
> type
> > > and
> > > >>> > making
> > > >>> > > > it
> > > >>> > > > > > > `@Public`) happened in the same release.
> > > >>> > > > > > >
> > > >>> > > > > > > However, those changes (`SourceReaderContext` and
> > > >>> > > > > `ResolvedCatalogTable`)
> > > >>> > > > > > > should have been clearly mentioned in the release notes
> > > with
> > > >>> an
> > > >>> > > > upgrade
> > > >>> > > > > > > guide.
> > > >>> > > > > > >
> > > >>> > > > > > > Best, Piotrek
> > > >>> > > > > > >
> > > >>> > > > > > > [1]
> > > >>> > > > > > >
> > > >>> > > > > >
> > > >>> > > > >
> > > >>> > > >
> > > >>> > >
> > > >>> >
> > > >>>
> > >
> >
> https://github.com/apache/flink/commit/7f3636f6b4f8bac415a7db85917ad849636bd730#diff-a247a24ccd1afc07c5d690a8a58b1f6584329925fdf0d7dc89361b90d621b7f2R31
> > > >>> > > > > > > [2] https://issues.apache.org/jira/browse/FLINK-22357
> > > >>> > > > > > >
> > > >>> > > > > > > wt., 28 wrz 2021 o 14:49 Leonard Xu <xbjtdcq@gmail.com
> >
> > > >>> > > napisał(a):
> > > >>> > > > > > >
> > > >>> > > > > > >>>>
> > > >>> > > > > > >>>> Not sure if this will happen in 1.15 already. We
> will
> > > >>> needed
> > > >>> > > > > automated
> > > >>> > > > > > >>>> compatibility tests and a well-defined list of
> stable
> > > API.
> > > >>> > > > > > >>
> > > >>> > > > > > >>> We are
> > > >>> > > > > > >>> trying to provide forward compatibility: applications
> > > using
> > > >>> > > > `@Public`
> > > >>> > > > > > >> APIs
> > > >>> > > > > > >>> compiled against Flink 1.12.x, should work fine in
> > Flink
> > > >>> 1.13.x
> > > >>> > > > > > >>
> > > >>> > > > > > >> Unfortunately, I also meet forward compatibility
> issue,
> > > >>> when I
> > > >>> > do
> > > >>> > > > the
> > > >>> > > > > > >> release 1.14 check, I try to use mysql-cdc
> connector[1]
> > > >>> which
> > > >>> > > > compiled
> > > >>> > > > > > >> against 1.13.1in SQL Client, but it can not work in
> > flink
> > > >>> 1.14.0
> > > >>> > > > > > cluster,
> > > >>> > > > > > >> it failed due to the metric API compatibility broken.
> > > >>> > > > > > >>
> > > >>> > > > > > >> @Public
> > > >>> > > > > > >> public interface SourceReaderContext {
> > > >>> > > > > > >>
> > > >>> > > > > > >> MetricGroup metricGroup();
> > > >>> > > > > > >>
> > > >>> > > > > > >>
> > > >>> > > > > > >> @Public
> > > >>> > > > > > >> public interface SourceReaderContext {
> > > >>> > > > > > >>
> > > >>> > > > > > >> SourceReaderMetricGroup metricGroup();
> > > >>> > > > > > >>
> > > >>> > > > > > >>
> > > >>> > > > > > >> Shouldn't we mark it as @Deprecated and then delete it
> > > util
> > > >>> > 2.0.0
> > > >>> > > > > > version
> > > >>> > > > > > >> for @Public API as the our community rule [2]
> described?
> > > At
> > > >>> > least
> > > >>> > > we
> > > >>> > > > > > should
> > > >>> > > > > > >> keep them across server minor versions
> > > >>> > (<major>.<minor>.<patch>).
> > > >>> > > > > > >>
> > > >>> > > > > > >> Although these changes can be tracked to voted FLIPs
> and
> > > >>> it’s
> > > >>> > not
> > > >>> > > > the
> > > >>> > > > > > >> fault of a few developers, it show us the fact that we
> > > >>> didn’t
> > > >>> > pay
> > > >>> > > > > enough
> > > >>> > > > > > >> attention to back compatibility/forward compatibility.
> > > >>> > > > > > >>
> > > >>> > > > > > >> Best,
> > > >>> > > > > > >> Leonard
> > > >>> > > > > > >> [1]
> > > >>> > > > > > >>
> > > >>> > > > > >
> > > >>> > > > >
> > > >>> > > >
> > > >>> > >
> > > >>> >
> > > >>>
> > >
> >
> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
> > > >>> > > > > > >> [2]
> > > >>> > > > > > >>
> > > >>> > > > >
> > > >>> > >
> > > >>>
> > >
> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> > > >>> > > > > > >>
> > > >>> > > > > > >>
> > > >>> > > > > >
> > > >>> > > > > >
> > > >>> > > > >
> > > >>> > > >
> > > >>> > >
> > > >>> >
> > > >>>
> > > >>
> > >
> >
>
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Arvid Heise <ar...@ververica.com>.
Hi Jark,
I also don't see it as a blocker issue at all. If you want to access the
metric group across 1.13 and 1.14, you can use
(MetricGroup) context.getClass().getMethod("metricGroup").invoke(context)
But of course, you will not be able to access the new standardized metrics.
For that you will need to maintain two different source/binary builds,
since it's a new feature.
I agree with Piotr, the issue is that we need a more standardized process
around PublicEvolving. Ideally, with every new minor release, we should
convert PublicEvolving to Public and Experimental to PublicEvolving. We
could extend the interfaces to capture a target version and a comment for
why the API is not Public yet. Before every release, we would go through
the annotated classes and either find a specific reason to keep the
annotation or move it towards Public. If we have a specific reason to keep
it Experimental/PublicEvolving, we should plan to address that reason with
the next release.
We do have good checks in place for Public; we are just too slow with
ensuring that new API becomes Public.
On Fri, Oct 1, 2021 at 9:41 PM Piotr Nowojski <pn...@apache.org> wrote:
> Hi,
>
> I don't understand why we are talking about this being a blocker issue? New
> sources were not marked as @Public for a good reason until 1.14. I agree,
> we should try better at making APIs @Public sooner. I was even proposing to
> create strict timeout rules (enforced via some automated checks) like
> (unless for a very very good reason) everything marked @PublicEvolving
> or @Experimental should be upgraded to @Public after for example 2 years
> [1]. But for example the new Sink API IMO is too fresh to make it
> `@Public`.
>
> It doesn't change the fact that if we could provide a compatibility layer
> between 1.13.x and 1.14.x for this SourceReaderContext issue, it would be a
> nice thing to do. I would be -1 for keeping it forever, but trying to
> support forward compatibility of `@PublicEvolving` APIs for one or two
> releases into the future might be a good rule of thumb.
>
> Best, Piotrek
>
> [1] "[DISCUSS] Dealing with deprecated and legacy code in Flink" on the dev
> mailing list
>
>
> pt., 1 paź 2021 o 16:56 Jark Wu <im...@gmail.com> napisał(a):
>
> > Hi Arvid,
> >
> > > Should we expect connector devs to release different connector binaries
> > for different Flink minors?
> > From the discussion of this thread, I think the answer is obviously
> "not",
> > otherwise OpenInx won't start
> > this discussion. As a maintainer of flink-cdc-connector, I have to say
> > that it's very hard to release
> > connectors for different flink versions. Usually, the connector
> community
> > doesn't have so much time to
> > maintain different branches/modules/code for different flink versions.
> >
> > > If we change it back, then a specific connector would work for 1.14.1
> and
> > 1.13.X but not for 1.14.0 and this would be even more confusing.
> > I think this is fine. IMO, this is a blocker issue of 1.14.0 which breaks
> > Source connectors.
> > We should suggest users to use 1.14.1 if they use Source connectors.
> >
> > Best,
> > Jark
> >
> >
> > On Fri, 1 Oct 2021 at 19:05, Arvid Heise <ar...@ververica.com> wrote:
> >
> > > The issue is that if we do not mark them as Public, we will always have
> > > incompatibilities. The change of SourceReaderContext#metricGroup is
> > > perfectly fine according to the annotation. The implications that we
> see
> > > here just mean that the interfaces have been expected to be Public.
> > >
> > > And now the question is what do we expect?
> > > Should we expect connector devs to release different connector binaries
> > > for different Flink minors? Then PublicEvolving is fine.
> > > If we expect that the same connector can work across multiple Flink
> > > versions, we need to go into Public.
> > >
> > > It doesn't make sense to keep them PublicEvolving on the annotation but
> > > implicitly assume them to be Public.
> > >
> > > @Jark Wu <im...@gmail.com> I don't see a way to revert the change of
> > > SourceReaderContext#metricGroup. For now, connector devs that expose
> > > metrics need to release 2 versions. If we change it back, then a
> specific
> > > connector would work for 1.14.1 and 1.13.X but not for 1.14.0 and this
> > > would be even more confusing.
> > >
> > > On Fri, Oct 1, 2021 at 10:49 AM Ingo Bürk <in...@ververica.com> wrote:
> > >
> > >> Hi,
> > >>
> > >> > [...] but also the new Source/Sink APIs as public
> > >>
> > >> I'm not really involved in the new Source/Sink APIs and will happily
> > >> listen to the developers working with them here, but since they are
> > new, we
> > >> should also be careful not to mark them as stable too quickly. We've
> > only
> > >> begun updating the existing connectors to these interfaces at the
> > moment.
> > >> Making more progress here and keeping new APIs as Evolving for a
> couple
> > of
> > >> minor releases is probably still a good idea. Maybe we should even
> have
> > >> actual rules on when APIs can/should be promoted?
> > >>
> > >> More actively checking backwards-compatibility during a release sounds
> > >> like a great idea regardless, of course.
> > >>
> > >>
> > >> Ingo
> > >>
> > >> On Fri, Oct 1, 2021 at 9:19 AM Jark Wu <im...@gmail.com> wrote:
> > >>
> > >>> Hi all,
> > >>>
> > >>> Nice thread and great discussion! Ecosystem is one of the most
> > important
> > >>> things
> > >>> to the Flink community, we should pay more attention to API
> > >>> compatibility.
> > >>>
> > >>> Marking all connector APIs @Public is a good idea, not only mark the
> > >>> Table/SQL
> > >>> connector APIs public, but also the new Source/Sink APIs as public.
> > >>> Besides, we should also add a check item to the Verify Release
> > >>> documentation[1]
> > >>> to verify the release is backward-compatible for connectors. From my
> > >>> point
> > >>> of view,
> > >>> such backward incompatibility should cancel the vote.
> > >>>
> > >>> Regarding the SourceReaderContext#metricGroup compatibility problem
> in
> > >>> 1.14.0, I would
> > >>> suggest starting a new discussion thread to see whether we have any
> > idea
> > >>> to
> > >>> fix it. We should
> > >>> fix it ASAP! Otherwise iceberg/hudi/cdc communities will get
> frustrated
> > >>> again when upgrading
> > >>> to 1.14. Maybe we still have time to release a minor version,
> because
> > >>> there is no
> > >>> connector upgraded to 1.14.0 yet. What do you think? @Leonard Xu
> > >>> <xb...@gmail.com> @Arvid Heise <ar...@ververica.com> @Piotr
> Nowojski
> > >>> <pi...@ververica.com>
> > >>>
> > >>> Best,
> > >>> Jark
> > >>>
> > >>> [1]:
> > >>>
> > >>>
> >
> https://cwiki.apache.org/confluence/display/FLINK/Verifying+a+Flink+Release
> > >>>
> > >>> On Wed, 29 Sept 2021 at 09:46, OpenInx <op...@gmail.com> wrote:
> > >>>
> > >>> > > Apart from this being `@PublicEvolving`
> > >>> >
> > >>> > From my perspective, annotating the 'DynamicTableSink' to be a
> > >>> > 'PublicEvolving' class is not reasonable, because that means devs
> > could
> > >>> > just change the basic API which all downstream connectors are
> > >>> depending on
> > >>> > easily when iterating flink from 1.12 to 1.13 (according to the
> wiki
> > >>> [1]).
> > >>> > This implies all downstream maintainers must take on this
> maintenance
> > >>> > burden, and it also makes our flink ecosystem very fragile.
> > Changing
> > >>> the
> > >>> > 'DynamicTableSink' between two major versions sounds reasonable to
> > me,
> > >>> but
> > >>> > unreasonable for uncompatibility changes between two minor
> versions.
> > >>> I
> > >>> > think we may need to check those API which are annotated
> > >>> 'PublicEnvoling'
> > >>> > while should be 'Public' because of the dependency from all
> > >>> connectors.
> > >>> > We should ensure the stability of those APIs that are necessary to
> > >>> > implement the connector, and at the same time implement the updated
> > v2
> > >>> > version of the API. After all v2 APIs are considered stable, we
> will
> > >>> mark
> > >>> > them as stable. Instead of releasing a version of the API, some of
> > the
> > >>> APIs
> > >>> > necessary to implement the connector are marked as stable and some
> > are
> > >>> > marked as unstable, which is very unfriendly to downstream. Because
> > >>> > downstream essentially every upgrade requires refactoring of the
> > code.
> > >>> >
> > >>> > > We are trying to provide forward compatibility: applications
> using
> > >>> > `@Public` APIs compiled against Flink 1.12.x, should work fine in
> > Flink
> > >>> > 1.13.x
> > >>> >
> > >>> > Thanks for clarifying this. Sounds reasonable to me, then we
> apache
> > >>> > iceberg could just use flink 1.12.x to build the flink+iceberg
> > >>> connector
> > >>> > and should make all the tests work fine for both flink 1.12 & flink
> > >>> 1.13.
> > >>> > For the `ResolvedCatalogTable` changes, I don't think it
> guarantees
> > >>> the
> > >>> > forward compatibility as you said, because the
> > >>> flink-iceberg-runtime.jar
> > >>> > compiled by flink 1.12 can still not works fine in flink 1.13
> because
> > >>> it
> > >>> > will need the flink1.12's `CatalogTable` to be cast to a flink1.13
> > 's
> > >>> > `ResolvedCatalogTable`. In general, I agree that forward
> > >>> compatibility is
> > >>> > a more clear compatibility guarantee.
> > >>> >
> > >>> > [1].
> > >>> >
> > >>>
> > https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> > >>> >
> > >>> >
> > >>> > On Tue, Sep 28, 2021 at 10:33 PM David Morávek <dm...@apache.org>
> > >>> wrote:
> > >>> >
> > >>> > > >
> > >>> > > > I think we have a compile time checks for breaking changes in
> > >>> `@Public`
> > >>> > > > marked classes/interfaces using japicmp [1].
> > >>> > >
> > >>> > >
> > >>> > > Nice, thanks for pointing that out, I'll take a closer look at it
> > ;)
> > >>> > >
> > >>> > > Best,
> > >>> > > D.
> > >>> > >
> > >>> > > On Tue, Sep 28, 2021 at 4:14 PM Piotr Nowojski <
> > pnowojski@apache.org
> > >>> >
> > >>> > > wrote:
> > >>> > >
> > >>> > > > > - We don't have any safeguards for stable API breaks. Big +1
> > for
> > >>> > Ingo's
> > >>> > > > effort with architectural tests [3].
> > >>> > > >
> > >>> > > > I think we have a compile time checks for breaking changes in
> > >>> `@Public`
> > >>> > > > marked classes/interfaces using japicmp [1].
> > >>> > > >
> > >>> > > > Piotrek
> > >>> > > >
> > >>> > > > [1]
> > >>> https://github.com/apache/flink/blob/master/pom.xml#L2014:L2084
> > >>> > > >
> > >>> > > > wt., 28 wrz 2021 o 15:59 David Morávek <dm...@apache.org>
> > >>> napisał(a):
> > >>> > > >
> > >>> > > > > This is a super interesting topic and there is already a
> great
> > >>> > > > discussion.
> > >>> > > > > Here are few thoughts:
> > >>> > > > >
> > >>> > > > > - There is a delicate balance between fast delivery of the
> new
> > >>> > features
> > >>> > > > and
> > >>> > > > > API stability. Even though we should be careful with breaking
> > >>> > evolving
> > >>> > > > > interfaces, it shouldn't stop us from making fast progress /
> > >>> iterate
> > >>> > on
> > >>> > > > > features.
> > >>> > > > > - There are two camps of users. One camp prefers more
> frequent
> > >>> > > releases /
> > >>> > > > > new features (early adopters) and second that prefer less
> > >>> frequent
> > >>> > > stable
> > >>> > > > > releases. There was already a great discussion about this at
> > >>> Flink
> > >>> > 1.14
> > >>> > > > > thread [1].
> > >>> > > > > - We're already trying to be explicit about which API's may
> > >>> break via
> > >>> > > > > annotations and the feature radar [2]. Stability annotations
> > are
> > >>> a
> > >>> > well
> > >>> > > > > known concept used by many projects. I think we still can go
> > bit
> > >>> > > further
> > >>> > > > > here and aim for an IDE support (for example usages of guava
> > >>> > > > @Experimental
> > >>> > > > > interfaces get highlighted, raising more awareness about
> > >>> potential
> > >>> > > > issues).
> > >>> > > > > I'm not sure how this IDE integration works though.
> > >>> > > > > - We don't have any safeguards for stable API breaks. Big +1
> > for
> > >>> > Ingo's
> > >>> > > > > effort with architectural tests [3].
> > >>> > > > >
> > >>> > > > > [1]
> > >>> > > > >
> > >>> > > > >
> > >>> > > >
> > >>> > >
> > >>> >
> > >>>
> >
> https://lists.apache.org/thread.html/r76e1cdba577c6f4d6c86b23fdaeb53c4e3744c20d0b3e850fc2e14a7%40%3Cdev.flink.apache.org%3E
> > >>> > > > > [2] https://flink.apache.org/roadmap.html
> > >>> > > > > [3] https://issues.apache.org/jira/browse/FLINK-24138
> > >>> > > > >
> > >>> > > > > Best,
> > >>> > > > > D.
> > >>> > > > >
> > >>> > > > > On Tue, Sep 28, 2021 at 3:49 PM Leonard Xu <
> xbjtdcq@gmail.com>
> > >>> > wrote:
> > >>> > > > >
> > >>> > > > > > Thanks Piotr for the kindly reply, what confused me is that
> > >>> > > > > > `SourceReaderContext` was marked @Public when it was born
> in
> > >>> flink
> > >>> > > > 1.11,
> > >>> > > > > > and then it was corrected to @PublicEvolving in 1.11 -_-,
> and
> > >>> > finally
> > >>> > > > it
> > >>> > > > > > was changed to @Public again...
> > >>> > > > > >
> > >>> > > > > > As Flink and Flink ecosystem(Flink CDC connectors)
> > developer, I
> > >>> > think
> > >>> > > > > what
> > >>> > > > > > we're discussing is meaningful and I’d like to help improve
> > >>> those
> > >>> > > > Public
> > >>> > > > > > API check for those changes.
> > >>> > > > > > Chesnay’s tips is a good idea that maybe we can use the
> tool
> > >>> like
> > >>> > > > japicmp
> > >>> > > > > > to do the check for every PR in CI phase.
> > >>> > > > > >
> > >>> > > > > > Best,
> > >>> > > > > > Leonard
> > >>> > > > > >
> > >>> > > > > >
> > >>> > > > > > > 在 2021年9月28日，21:15，Piotr Nowojski <pn...@apache.org>
> > 写道：
> > >>> > > > > > >
> > >>> > > > > > > Hi Leonard,
> > >>> > > > > > >
> > >>> > > > > > > Sorry for this causing you troubles, however that change
> in
> > >>> the
> > >>> > > > return
> > >>> > > > > > type
> > >>> > > > > > > was done while this class still has been marked as
> > >>> > > > > `@PublicEvolving`[1].
> > >>> > > > > > As
> > >>> > > > > > > of 1.13.x `SourceReaderContext` was `@PublicEvolving` and
> > it
> > >>> was
> > >>> > > > marked
> > >>> > > > > > as
> > >>> > > > > > > `@Public` only starting from Flink 1.14.0 [2]. Probably
> > what
> > >>> > > confused
> > >>> > > > > you
> > >>> > > > > > > was that both of those changes (changing the return type
> > and
> > >>> > making
> > >>> > > > it
> > >>> > > > > > > `@Public`) happened in the same release.
> > >>> > > > > > >
> > >>> > > > > > > However, those changes (`SourceReaderContext` and
> > >>> > > > > `ResolvedCatalogTable`)
> > >>> > > > > > > should have been clearly mentioned in the release notes
> > with
> > >>> an
> > >>> > > > upgrade
> > >>> > > > > > > guide.
> > >>> > > > > > >
> > >>> > > > > > > Best, Piotrek
> > >>> > > > > > >
> > >>> > > > > > > [1]
> > >>> > > > > > >
> > >>> > > > > >
> > >>> > > > >
> > >>> > > >
> > >>> > >
> > >>> >
> > >>>
> >
> https://github.com/apache/flink/commit/7f3636f6b4f8bac415a7db85917ad849636bd730#diff-a247a24ccd1afc07c5d690a8a58b1f6584329925fdf0d7dc89361b90d621b7f2R31
> > >>> > > > > > > [2] https://issues.apache.org/jira/browse/FLINK-22357
> > >>> > > > > > >
> > >>> > > > > > > wt., 28 wrz 2021 o 14:49 Leonard Xu <xb...@gmail.com>
> > >>> > > napisał(a):
> > >>> > > > > > >
> > >>> > > > > > >>>>
> > >>> > > > > > >>>> Not sure if this will happen in 1.15 already. We will
> > >>> needed
> > >>> > > > > automated
> > >>> > > > > > >>>> compatibility tests and a well-defined list of stable
> > API.
> > >>> > > > > > >>
> > >>> > > > > > >>> We are
> > >>> > > > > > >>> trying to provide forward compatibility: applications
> > using
> > >>> > > > `@Public`
> > >>> > > > > > >> APIs
> > >>> > > > > > >>> compiled against Flink 1.12.x, should work fine in
> Flink
> > >>> 1.13.x
> > >>> > > > > > >>
> > >>> > > > > > >> Unfortunately, I also meet forward compatibility issue,
> > >>> when I
> > >>> > do
> > >>> > > > the
> > >>> > > > > > >> release 1.14 check, I try to use mysql-cdc connector[1]
> > >>> which
> > >>> > > > compiled
> > >>> > > > > > >> against 1.13.1in SQL Client, but it can not work in
> flink
> > >>> 1.14.0
> > >>> > > > > > cluster,
> > >>> > > > > > >> it failed due to the metric API compatibility broken.
> > >>> > > > > > >>
> > >>> > > > > > >> @Public
> > >>> > > > > > >> public interface SourceReaderContext {
> > >>> > > > > > >>
> > >>> > > > > > >> MetricGroup metricGroup();
> > >>> > > > > > >>
> > >>> > > > > > >>
> > >>> > > > > > >> @Public
> > >>> > > > > > >> public interface SourceReaderContext {
> > >>> > > > > > >>
> > >>> > > > > > >> SourceReaderMetricGroup metricGroup();
> > >>> > > > > > >>
> > >>> > > > > > >>
> > >>> > > > > > >> Shouldn't we mark it as @Deprecated and then delete it
> > util
> > >>> > 2.0.0
> > >>> > > > > > version
> > >>> > > > > > >> for @Public API as the our community rule [2] described?
> > At
> > >>> > least
> > >>> > > we
> > >>> > > > > > should
> > >>> > > > > > >> keep them across server minor versions
> > >>> > (<major>.<minor>.<patch>).
> > >>> > > > > > >>
> > >>> > > > > > >> Although these changes can be tracked to voted FLIPs and
> > >>> it’s
> > >>> > not
> > >>> > > > the
> > >>> > > > > > >> fault of a few developers, it show us the fact that we
> > >>> didn’t
> > >>> > pay
> > >>> > > > > enough
> > >>> > > > > > >> attention to back compatibility/forward compatibility.
> > >>> > > > > > >>
> > >>> > > > > > >> Best,
> > >>> > > > > > >> Leonard
> > >>> > > > > > >> [1]
> > >>> > > > > > >>
> > >>> > > > > >
> > >>> > > > >
> > >>> > > >
> > >>> > >
> > >>> >
> > >>>
> >
> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
> > >>> > > > > > >> [2]
> > >>> > > > > > >>
> > >>> > > > >
> > >>> > >
> > >>>
> > https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> > >>> > > > > > >>
> > >>> > > > > > >>
> > >>> > > > > >
> > >>> > > > > >
> > >>> > > > >
> > >>> > > >
> > >>> > >
> > >>> >
> > >>>
> > >>
> >
>
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Piotr Nowojski <pn...@apache.org>.
Hi,
I don't understand why we are talking about this being a blocker issue? New
sources were not marked as @Public for a good reason until 1.14. I agree,
we should try better at making APIs @Public sooner. I was even proposing to
create strict timeout rules (enforced via some automated checks) like
(unless for a very very good reason) everything marked @PublicEvolving
or @Experimental should be upgraded to @Public after for example 2 years
[1]. But for example the new Sink API IMO is too fresh to make it `@Public`.
It doesn't change the fact that if we could provide a compatibility layer
between 1.13.x and 1.14.x for this SourceReaderContext issue, it would be a
nice thing to do. I would be -1 for keeping it forever, but trying to
support forward compatibility of `@PublicEvolving` APIs for one or two
releases into the future might be a good rule of thumb.
Best, Piotrek
[1] "[DISCUSS] Dealing with deprecated and legacy code in Flink" on the dev
mailing list
pt., 1 paź 2021 o 16:56 Jark Wu <im...@gmail.com> napisał(a):
> Hi Arvid,
>
> > Should we expect connector devs to release different connector binaries
> for different Flink minors?
> From the discussion of this thread, I think the answer is obviously "not",
> otherwise OpenInx won't start
> this discussion. As a maintainer of flink-cdc-connector, I have to say
> that it's very hard to release
> connectors for different flink versions. Usually, the connector community
> doesn't have so much time to
> maintain different branches/modules/code for different flink versions.
>
> > If we change it back, then a specific connector would work for 1.14.1 and
> 1.13.X but not for 1.14.0 and this would be even more confusing.
> I think this is fine. IMO, this is a blocker issue of 1.14.0 which breaks
> Source connectors.
> We should suggest users to use 1.14.1 if they use Source connectors.
>
> Best,
> Jark
>
>
> On Fri, 1 Oct 2021 at 19:05, Arvid Heise <ar...@ververica.com> wrote:
>
> > The issue is that if we do not mark them as Public, we will always have
> > incompatibilities. The change of SourceReaderContext#metricGroup is
> > perfectly fine according to the annotation. The implications that we see
> > here just mean that the interfaces have been expected to be Public.
> >
> > And now the question is what do we expect?
> > Should we expect connector devs to release different connector binaries
> > for different Flink minors? Then PublicEvolving is fine.
> > If we expect that the same connector can work across multiple Flink
> > versions, we need to go into Public.
> >
> > It doesn't make sense to keep them PublicEvolving on the annotation but
> > implicitly assume them to be Public.
> >
> > @Jark Wu <im...@gmail.com> I don't see a way to revert the change of
> > SourceReaderContext#metricGroup. For now, connector devs that expose
> > metrics need to release 2 versions. If we change it back, then a specific
> > connector would work for 1.14.1 and 1.13.X but not for 1.14.0 and this
> > would be even more confusing.
> >
> > On Fri, Oct 1, 2021 at 10:49 AM Ingo Bürk <in...@ververica.com> wrote:
> >
> >> Hi,
> >>
> >> > [...] but also the new Source/Sink APIs as public
> >>
> >> I'm not really involved in the new Source/Sink APIs and will happily
> >> listen to the developers working with them here, but since they are
> new, we
> >> should also be careful not to mark them as stable too quickly. We've
> only
> >> begun updating the existing connectors to these interfaces at the
> moment.
> >> Making more progress here and keeping new APIs as Evolving for a couple
> of
> >> minor releases is probably still a good idea. Maybe we should even have
> >> actual rules on when APIs can/should be promoted?
> >>
> >> More actively checking backwards-compatibility during a release sounds
> >> like a great idea regardless, of course.
> >>
> >>
> >> Ingo
> >>
> >> On Fri, Oct 1, 2021 at 9:19 AM Jark Wu <im...@gmail.com> wrote:
> >>
> >>> Hi all,
> >>>
> >>> Nice thread and great discussion! Ecosystem is one of the most
> important
> >>> things
> >>> to the Flink community, we should pay more attention to API
> >>> compatibility.
> >>>
> >>> Marking all connector APIs @Public is a good idea, not only mark the
> >>> Table/SQL
> >>> connector APIs public, but also the new Source/Sink APIs as public.
> >>> Besides, we should also add a check item to the Verify Release
> >>> documentation[1]
> >>> to verify the release is backward-compatible for connectors. From my
> >>> point
> >>> of view,
> >>> such backward incompatibility should cancel the vote.
> >>>
> >>> Regarding the SourceReaderContext#metricGroup compatibility problem in
> >>> 1.14.0, I would
> >>> suggest starting a new discussion thread to see whether we have any
> idea
> >>> to
> >>> fix it. We should
> >>> fix it ASAP! Otherwise iceberg/hudi/cdc communities will get frustrated
> >>> again when upgrading
> >>> to 1.14. Maybe we still have time to release a minor version, because
> >>> there is no
> >>> connector upgraded to 1.14.0 yet. What do you think? @Leonard Xu
> >>> <xb...@gmail.com> @Arvid Heise <ar...@ververica.com> @Piotr Nowojski
> >>> <pi...@ververica.com>
> >>>
> >>> Best,
> >>> Jark
> >>>
> >>> [1]:
> >>>
> >>>
> https://cwiki.apache.org/confluence/display/FLINK/Verifying+a+Flink+Release
> >>>
> >>> On Wed, 29 Sept 2021 at 09:46, OpenInx <op...@gmail.com> wrote:
> >>>
> >>> > > Apart from this being `@PublicEvolving`
> >>> >
> >>> > From my perspective, annotating the 'DynamicTableSink' to be a
> >>> > 'PublicEvolving' class is not reasonable, because that means devs
> could
> >>> > just change the basic API which all downstream connectors are
> >>> depending on
> >>> > easily when iterating flink from 1.12 to 1.13 (according to the wiki
> >>> [1]).
> >>> > This implies all downstream maintainers must take on this maintenance
> >>> > burden, and it also makes our flink ecosystem very fragile.
> Changing
> >>> the
> >>> > 'DynamicTableSink' between two major versions sounds reasonable to
> me,
> >>> but
> >>> > unreasonable for uncompatibility changes between two minor versions.
> >>> I
> >>> > think we may need to check those API which are annotated
> >>> 'PublicEnvoling'
> >>> > while should be 'Public' because of the dependency from all
> >>> connectors.
> >>> > We should ensure the stability of those APIs that are necessary to
> >>> > implement the connector, and at the same time implement the updated
> v2
> >>> > version of the API. After all v2 APIs are considered stable, we will
> >>> mark
> >>> > them as stable. Instead of releasing a version of the API, some of
> the
> >>> APIs
> >>> > necessary to implement the connector are marked as stable and some
> are
> >>> > marked as unstable, which is very unfriendly to downstream. Because
> >>> > downstream essentially every upgrade requires refactoring of the
> code.
> >>> >
> >>> > > We are trying to provide forward compatibility: applications using
> >>> > `@Public` APIs compiled against Flink 1.12.x, should work fine in
> Flink
> >>> > 1.13.x
> >>> >
> >>> > Thanks for clarifying this. Sounds reasonable to me, then we apache
> >>> > iceberg could just use flink 1.12.x to build the flink+iceberg
> >>> connector
> >>> > and should make all the tests work fine for both flink 1.12 & flink
> >>> 1.13.
> >>> > For the `ResolvedCatalogTable` changes, I don't think it guarantees
> >>> the
> >>> > forward compatibility as you said, because the
> >>> flink-iceberg-runtime.jar
> >>> > compiled by flink 1.12 can still not works fine in flink 1.13 because
> >>> it
> >>> > will need the flink1.12's `CatalogTable` to be cast to a flink1.13
> 's
> >>> > `ResolvedCatalogTable`. In general, I agree that forward
> >>> compatibility is
> >>> > a more clear compatibility guarantee.
> >>> >
> >>> > [1].
> >>> >
> >>>
> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> >>> >
> >>> >
> >>> > On Tue, Sep 28, 2021 at 10:33 PM David Morávek <dm...@apache.org>
> >>> wrote:
> >>> >
> >>> > > >
> >>> > > > I think we have a compile time checks for breaking changes in
> >>> `@Public`
> >>> > > > marked classes/interfaces using japicmp [1].
> >>> > >
> >>> > >
> >>> > > Nice, thanks for pointing that out, I'll take a closer look at it
> ;)
> >>> > >
> >>> > > Best,
> >>> > > D.
> >>> > >
> >>> > > On Tue, Sep 28, 2021 at 4:14 PM Piotr Nowojski <
> pnowojski@apache.org
> >>> >
> >>> > > wrote:
> >>> > >
> >>> > > > > - We don't have any safeguards for stable API breaks. Big +1
> for
> >>> > Ingo's
> >>> > > > effort with architectural tests [3].
> >>> > > >
> >>> > > > I think we have a compile time checks for breaking changes in
> >>> `@Public`
> >>> > > > marked classes/interfaces using japicmp [1].
> >>> > > >
> >>> > > > Piotrek
> >>> > > >
> >>> > > > [1]
> >>> https://github.com/apache/flink/blob/master/pom.xml#L2014:L2084
> >>> > > >
> >>> > > > wt., 28 wrz 2021 o 15:59 David Morávek <dm...@apache.org>
> >>> napisał(a):
> >>> > > >
> >>> > > > > This is a super interesting topic and there is already a great
> >>> > > > discussion.
> >>> > > > > Here are few thoughts:
> >>> > > > >
> >>> > > > > - There is a delicate balance between fast delivery of the new
> >>> > features
> >>> > > > and
> >>> > > > > API stability. Even though we should be careful with breaking
> >>> > evolving
> >>> > > > > interfaces, it shouldn't stop us from making fast progress /
> >>> iterate
> >>> > on
> >>> > > > > features.
> >>> > > > > - There are two camps of users. One camp prefers more frequent
> >>> > > releases /
> >>> > > > > new features (early adopters) and second that prefer less
> >>> frequent
> >>> > > stable
> >>> > > > > releases. There was already a great discussion about this at
> >>> Flink
> >>> > 1.14
> >>> > > > > thread [1].
> >>> > > > > - We're already trying to be explicit about which API's may
> >>> break via
> >>> > > > > annotations and the feature radar [2]. Stability annotations
> are
> >>> a
> >>> > well
> >>> > > > > known concept used by many projects. I think we still can go
> bit
> >>> > > further
> >>> > > > > here and aim for an IDE support (for example usages of guava
> >>> > > > @Experimental
> >>> > > > > interfaces get highlighted, raising more awareness about
> >>> potential
> >>> > > > issues).
> >>> > > > > I'm not sure how this IDE integration works though.
> >>> > > > > - We don't have any safeguards for stable API breaks. Big +1
> for
> >>> > Ingo's
> >>> > > > > effort with architectural tests [3].
> >>> > > > >
> >>> > > > > [1]
> >>> > > > >
> >>> > > > >
> >>> > > >
> >>> > >
> >>> >
> >>>
> https://lists.apache.org/thread.html/r76e1cdba577c6f4d6c86b23fdaeb53c4e3744c20d0b3e850fc2e14a7%40%3Cdev.flink.apache.org%3E
> >>> > > > > [2] https://flink.apache.org/roadmap.html
> >>> > > > > [3] https://issues.apache.org/jira/browse/FLINK-24138
> >>> > > > >
> >>> > > > > Best,
> >>> > > > > D.
> >>> > > > >
> >>> > > > > On Tue, Sep 28, 2021 at 3:49 PM Leonard Xu <xb...@gmail.com>
> >>> > wrote:
> >>> > > > >
> >>> > > > > > Thanks Piotr for the kindly reply, what confused me is that
> >>> > > > > > `SourceReaderContext` was marked @Public when it was born in
> >>> flink
> >>> > > > 1.11,
> >>> > > > > > and then it was corrected to @PublicEvolving in 1.11 -_-, and
> >>> > finally
> >>> > > > it
> >>> > > > > > was changed to @Public again...
> >>> > > > > >
> >>> > > > > > As Flink and Flink ecosystem(Flink CDC connectors)
> developer, I
> >>> > think
> >>> > > > > what
> >>> > > > > > we're discussing is meaningful and I’d like to help improve
> >>> those
> >>> > > > Public
> >>> > > > > > API check for those changes.
> >>> > > > > > Chesnay’s tips is a good idea that maybe we can use the tool
> >>> like
> >>> > > > japicmp
> >>> > > > > > to do the check for every PR in CI phase.
> >>> > > > > >
> >>> > > > > > Best,
> >>> > > > > > Leonard
> >>> > > > > >
> >>> > > > > >
> >>> > > > > > > 在 2021年9月28日，21:15，Piotr Nowojski <pn...@apache.org>
> 写道：
> >>> > > > > > >
> >>> > > > > > > Hi Leonard,
> >>> > > > > > >
> >>> > > > > > > Sorry for this causing you troubles, however that change in
> >>> the
> >>> > > > return
> >>> > > > > > type
> >>> > > > > > > was done while this class still has been marked as
> >>> > > > > `@PublicEvolving`[1].
> >>> > > > > > As
> >>> > > > > > > of 1.13.x `SourceReaderContext` was `@PublicEvolving` and
> it
> >>> was
> >>> > > > marked
> >>> > > > > > as
> >>> > > > > > > `@Public` only starting from Flink 1.14.0 [2]. Probably
> what
> >>> > > confused
> >>> > > > > you
> >>> > > > > > > was that both of those changes (changing the return type
> and
> >>> > making
> >>> > > > it
> >>> > > > > > > `@Public`) happened in the same release.
> >>> > > > > > >
> >>> > > > > > > However, those changes (`SourceReaderContext` and
> >>> > > > > `ResolvedCatalogTable`)
> >>> > > > > > > should have been clearly mentioned in the release notes
> with
> >>> an
> >>> > > > upgrade
> >>> > > > > > > guide.
> >>> > > > > > >
> >>> > > > > > > Best, Piotrek
> >>> > > > > > >
> >>> > > > > > > [1]
> >>> > > > > > >
> >>> > > > > >
> >>> > > > >
> >>> > > >
> >>> > >
> >>> >
> >>>
> https://github.com/apache/flink/commit/7f3636f6b4f8bac415a7db85917ad849636bd730#diff-a247a24ccd1afc07c5d690a8a58b1f6584329925fdf0d7dc89361b90d621b7f2R31
> >>> > > > > > > [2] https://issues.apache.org/jira/browse/FLINK-22357
> >>> > > > > > >
> >>> > > > > > > wt., 28 wrz 2021 o 14:49 Leonard Xu <xb...@gmail.com>
> >>> > > napisał(a):
> >>> > > > > > >
> >>> > > > > > >>>>
> >>> > > > > > >>>> Not sure if this will happen in 1.15 already. We will
> >>> needed
> >>> > > > > automated
> >>> > > > > > >>>> compatibility tests and a well-defined list of stable
> API.
> >>> > > > > > >>
> >>> > > > > > >>> We are
> >>> > > > > > >>> trying to provide forward compatibility: applications
> using
> >>> > > > `@Public`
> >>> > > > > > >> APIs
> >>> > > > > > >>> compiled against Flink 1.12.x, should work fine in Flink
> >>> 1.13.x
> >>> > > > > > >>
> >>> > > > > > >> Unfortunately, I also meet forward compatibility issue,
> >>> when I
> >>> > do
> >>> > > > the
> >>> > > > > > >> release 1.14 check, I try to use mysql-cdc connector[1]
> >>> which
> >>> > > > compiled
> >>> > > > > > >> against 1.13.1in SQL Client, but it can not work in flink
> >>> 1.14.0
> >>> > > > > > cluster,
> >>> > > > > > >> it failed due to the metric API compatibility broken.
> >>> > > > > > >>
> >>> > > > > > >> @Public
> >>> > > > > > >> public interface SourceReaderContext {
> >>> > > > > > >>
> >>> > > > > > >> MetricGroup metricGroup();
> >>> > > > > > >>
> >>> > > > > > >>
> >>> > > > > > >> @Public
> >>> > > > > > >> public interface SourceReaderContext {
> >>> > > > > > >>
> >>> > > > > > >> SourceReaderMetricGroup metricGroup();
> >>> > > > > > >>
> >>> > > > > > >>
> >>> > > > > > >> Shouldn't we mark it as @Deprecated and then delete it
> util
> >>> > 2.0.0
> >>> > > > > > version
> >>> > > > > > >> for @Public API as the our community rule [2] described?
> At
> >>> > least
> >>> > > we
> >>> > > > > > should
> >>> > > > > > >> keep them across server minor versions
> >>> > (<major>.<minor>.<patch>).
> >>> > > > > > >>
> >>> > > > > > >> Although these changes can be tracked to voted FLIPs and
> >>> it’s
> >>> > not
> >>> > > > the
> >>> > > > > > >> fault of a few developers, it show us the fact that we
> >>> didn’t
> >>> > pay
> >>> > > > > enough
> >>> > > > > > >> attention to back compatibility/forward compatibility.
> >>> > > > > > >>
> >>> > > > > > >> Best,
> >>> > > > > > >> Leonard
> >>> > > > > > >> [1]
> >>> > > > > > >>
> >>> > > > > >
> >>> > > > >
> >>> > > >
> >>> > >
> >>> >
> >>>
> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
> >>> > > > > > >> [2]
> >>> > > > > > >>
> >>> > > > >
> >>> > >
> >>>
> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> >>> > > > > > >>
> >>> > > > > > >>
> >>> > > > > >
> >>> > > > > >
> >>> > > > >
> >>> > > >
> >>> > >
> >>> >
> >>>
> >>
>
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Jark Wu <im...@gmail.com>.
Hi Arvid,
> Should we expect connector devs to release different connector binaries
for different Flink minors?
From the discussion of this thread, I think the answer is obviously "not",
otherwise OpenInx won't start
this discussion. As a maintainer of flink-cdc-connector, I have to say
that it's very hard to release
connectors for different flink versions. Usually, the connector community
doesn't have so much time to
maintain different branches/modules/code for different flink versions.
> If we change it back, then a specific connector would work for 1.14.1 and
1.13.X but not for 1.14.0 and this would be even more confusing.
I think this is fine. IMO, this is a blocker issue of 1.14.0 which breaks
Source connectors.
We should suggest users to use 1.14.1 if they use Source connectors.
Best,
Jark
On Fri, 1 Oct 2021 at 19:05, Arvid Heise <ar...@ververica.com> wrote:
> The issue is that if we do not mark them as Public, we will always have
> incompatibilities. The change of SourceReaderContext#metricGroup is
> perfectly fine according to the annotation. The implications that we see
> here just mean that the interfaces have been expected to be Public.
>
> And now the question is what do we expect?
> Should we expect connector devs to release different connector binaries
> for different Flink minors? Then PublicEvolving is fine.
> If we expect that the same connector can work across multiple Flink
> versions, we need to go into Public.
>
> It doesn't make sense to keep them PublicEvolving on the annotation but
> implicitly assume them to be Public.
>
> @Jark Wu <im...@gmail.com> I don't see a way to revert the change of
> SourceReaderContext#metricGroup. For now, connector devs that expose
> metrics need to release 2 versions. If we change it back, then a specific
> connector would work for 1.14.1 and 1.13.X but not for 1.14.0 and this
> would be even more confusing.
>
> On Fri, Oct 1, 2021 at 10:49 AM Ingo Bürk <in...@ververica.com> wrote:
>
>> Hi,
>>
>> > [...] but also the new Source/Sink APIs as public
>>
>> I'm not really involved in the new Source/Sink APIs and will happily
>> listen to the developers working with them here, but since they are new, we
>> should also be careful not to mark them as stable too quickly. We've only
>> begun updating the existing connectors to these interfaces at the moment.
>> Making more progress here and keeping new APIs as Evolving for a couple of
>> minor releases is probably still a good idea. Maybe we should even have
>> actual rules on when APIs can/should be promoted?
>>
>> More actively checking backwards-compatibility during a release sounds
>> like a great idea regardless, of course.
>>
>>
>> Ingo
>>
>> On Fri, Oct 1, 2021 at 9:19 AM Jark Wu <im...@gmail.com> wrote:
>>
>>> Hi all,
>>>
>>> Nice thread and great discussion! Ecosystem is one of the most important
>>> things
>>> to the Flink community, we should pay more attention to API
>>> compatibility.
>>>
>>> Marking all connector APIs @Public is a good idea, not only mark the
>>> Table/SQL
>>> connector APIs public, but also the new Source/Sink APIs as public.
>>> Besides, we should also add a check item to the Verify Release
>>> documentation[1]
>>> to verify the release is backward-compatible for connectors. From my
>>> point
>>> of view,
>>> such backward incompatibility should cancel the vote.
>>>
>>> Regarding the SourceReaderContext#metricGroup compatibility problem in
>>> 1.14.0, I would
>>> suggest starting a new discussion thread to see whether we have any idea
>>> to
>>> fix it. We should
>>> fix it ASAP! Otherwise iceberg/hudi/cdc communities will get frustrated
>>> again when upgrading
>>> to 1.14. Maybe we still have time to release a minor version, because
>>> there is no
>>> connector upgraded to 1.14.0 yet. What do you think? @Leonard Xu
>>> <xb...@gmail.com> @Arvid Heise <ar...@ververica.com> @Piotr Nowojski
>>> <pi...@ververica.com>
>>>
>>> Best,
>>> Jark
>>>
>>> [1]:
>>>
>>> https://cwiki.apache.org/confluence/display/FLINK/Verifying+a+Flink+Release
>>>
>>> On Wed, 29 Sept 2021 at 09:46, OpenInx <op...@gmail.com> wrote:
>>>
>>> > > Apart from this being `@PublicEvolving`
>>> >
>>> > From my perspective, annotating the 'DynamicTableSink' to be a
>>> > 'PublicEvolving' class is not reasonable, because that means devs could
>>> > just change the basic API which all downstream connectors are
>>> depending on
>>> > easily when iterating flink from 1.12 to 1.13 (according to the wiki
>>> [1]).
>>> > This implies all downstream maintainers must take on this maintenance
>>> > burden, and it also makes our flink ecosystem very fragile. Changing
>>> the
>>> > 'DynamicTableSink' between two major versions sounds reasonable to me,
>>> but
>>> > unreasonable for uncompatibility changes between two minor versions.
>>> I
>>> > think we may need to check those API which are annotated
>>> 'PublicEnvoling'
>>> > while should be 'Public' because of the dependency from all
>>> connectors.
>>> > We should ensure the stability of those APIs that are necessary to
>>> > implement the connector, and at the same time implement the updated v2
>>> > version of the API. After all v2 APIs are considered stable, we will
>>> mark
>>> > them as stable. Instead of releasing a version of the API, some of the
>>> APIs
>>> > necessary to implement the connector are marked as stable and some are
>>> > marked as unstable, which is very unfriendly to downstream. Because
>>> > downstream essentially every upgrade requires refactoring of the code.
>>> >
>>> > > We are trying to provide forward compatibility: applications using
>>> > `@Public` APIs compiled against Flink 1.12.x, should work fine in Flink
>>> > 1.13.x
>>> >
>>> > Thanks for clarifying this. Sounds reasonable to me, then we apache
>>> > iceberg could just use flink 1.12.x to build the flink+iceberg
>>> connector
>>> > and should make all the tests work fine for both flink 1.12 & flink
>>> 1.13.
>>> > For the `ResolvedCatalogTable` changes, I don't think it guarantees
>>> the
>>> > forward compatibility as you said, because the
>>> flink-iceberg-runtime.jar
>>> > compiled by flink 1.12 can still not works fine in flink 1.13 because
>>> it
>>> > will need the flink1.12's `CatalogTable` to be cast to a flink1.13 's
>>> > `ResolvedCatalogTable`. In general, I agree that forward
>>> compatibility is
>>> > a more clear compatibility guarantee.
>>> >
>>> > [1].
>>> >
>>> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
>>> >
>>> >
>>> > On Tue, Sep 28, 2021 at 10:33 PM David Morávek <dm...@apache.org>
>>> wrote:
>>> >
>>> > > >
>>> > > > I think we have a compile time checks for breaking changes in
>>> `@Public`
>>> > > > marked classes/interfaces using japicmp [1].
>>> > >
>>> > >
>>> > > Nice, thanks for pointing that out, I'll take a closer look at it ;)
>>> > >
>>> > > Best,
>>> > > D.
>>> > >
>>> > > On Tue, Sep 28, 2021 at 4:14 PM Piotr Nowojski <pnowojski@apache.org
>>> >
>>> > > wrote:
>>> > >
>>> > > > > - We don't have any safeguards for stable API breaks. Big +1 for
>>> > Ingo's
>>> > > > effort with architectural tests [3].
>>> > > >
>>> > > > I think we have a compile time checks for breaking changes in
>>> `@Public`
>>> > > > marked classes/interfaces using japicmp [1].
>>> > > >
>>> > > > Piotrek
>>> > > >
>>> > > > [1]
>>> https://github.com/apache/flink/blob/master/pom.xml#L2014:L2084
>>> > > >
>>> > > > wt., 28 wrz 2021 o 15:59 David Morávek <dm...@apache.org>
>>> napisał(a):
>>> > > >
>>> > > > > This is a super interesting topic and there is already a great
>>> > > > discussion.
>>> > > > > Here are few thoughts:
>>> > > > >
>>> > > > > - There is a delicate balance between fast delivery of the new
>>> > features
>>> > > > and
>>> > > > > API stability. Even though we should be careful with breaking
>>> > evolving
>>> > > > > interfaces, it shouldn't stop us from making fast progress /
>>> iterate
>>> > on
>>> > > > > features.
>>> > > > > - There are two camps of users. One camp prefers more frequent
>>> > > releases /
>>> > > > > new features (early adopters) and second that prefer less
>>> frequent
>>> > > stable
>>> > > > > releases. There was already a great discussion about this at
>>> Flink
>>> > 1.14
>>> > > > > thread [1].
>>> > > > > - We're already trying to be explicit about which API's may
>>> break via
>>> > > > > annotations and the feature radar [2]. Stability annotations are
>>> a
>>> > well
>>> > > > > known concept used by many projects. I think we still can go bit
>>> > > further
>>> > > > > here and aim for an IDE support (for example usages of guava
>>> > > > @Experimental
>>> > > > > interfaces get highlighted, raising more awareness about
>>> potential
>>> > > > issues).
>>> > > > > I'm not sure how this IDE integration works though.
>>> > > > > - We don't have any safeguards for stable API breaks. Big +1 for
>>> > Ingo's
>>> > > > > effort with architectural tests [3].
>>> > > > >
>>> > > > > [1]
>>> > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>> https://lists.apache.org/thread.html/r76e1cdba577c6f4d6c86b23fdaeb53c4e3744c20d0b3e850fc2e14a7%40%3Cdev.flink.apache.org%3E
>>> > > > > [2] https://flink.apache.org/roadmap.html
>>> > > > > [3] https://issues.apache.org/jira/browse/FLINK-24138
>>> > > > >
>>> > > > > Best,
>>> > > > > D.
>>> > > > >
>>> > > > > On Tue, Sep 28, 2021 at 3:49 PM Leonard Xu <xb...@gmail.com>
>>> > wrote:
>>> > > > >
>>> > > > > > Thanks Piotr for the kindly reply, what confused me is that
>>> > > > > > `SourceReaderContext` was marked @Public when it was born in
>>> flink
>>> > > > 1.11,
>>> > > > > > and then it was corrected to @PublicEvolving in 1.11 -_-, and
>>> > finally
>>> > > > it
>>> > > > > > was changed to @Public again...
>>> > > > > >
>>> > > > > > As Flink and Flink ecosystem(Flink CDC connectors) developer, I
>>> > think
>>> > > > > what
>>> > > > > > we're discussing is meaningful and I’d like to help improve
>>> those
>>> > > > Public
>>> > > > > > API check for those changes.
>>> > > > > > Chesnay’s tips is a good idea that maybe we can use the tool
>>> like
>>> > > > japicmp
>>> > > > > > to do the check for every PR in CI phase.
>>> > > > > >
>>> > > > > > Best,
>>> > > > > > Leonard
>>> > > > > >
>>> > > > > >
>>> > > > > > > 在 2021年9月28日，21:15，Piotr Nowojski <pn...@apache.org> 写道：
>>> > > > > > >
>>> > > > > > > Hi Leonard,
>>> > > > > > >
>>> > > > > > > Sorry for this causing you troubles, however that change in
>>> the
>>> > > > return
>>> > > > > > type
>>> > > > > > > was done while this class still has been marked as
>>> > > > > `@PublicEvolving`[1].
>>> > > > > > As
>>> > > > > > > of 1.13.x `SourceReaderContext` was `@PublicEvolving` and it
>>> was
>>> > > > marked
>>> > > > > > as
>>> > > > > > > `@Public` only starting from Flink 1.14.0 [2]. Probably what
>>> > > confused
>>> > > > > you
>>> > > > > > > was that both of those changes (changing the return type and
>>> > making
>>> > > > it
>>> > > > > > > `@Public`) happened in the same release.
>>> > > > > > >
>>> > > > > > > However, those changes (`SourceReaderContext` and
>>> > > > > `ResolvedCatalogTable`)
>>> > > > > > > should have been clearly mentioned in the release notes with
>>> an
>>> > > > upgrade
>>> > > > > > > guide.
>>> > > > > > >
>>> > > > > > > Best, Piotrek
>>> > > > > > >
>>> > > > > > > [1]
>>> > > > > > >
>>> > > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>> https://github.com/apache/flink/commit/7f3636f6b4f8bac415a7db85917ad849636bd730#diff-a247a24ccd1afc07c5d690a8a58b1f6584329925fdf0d7dc89361b90d621b7f2R31
>>> > > > > > > [2] https://issues.apache.org/jira/browse/FLINK-22357
>>> > > > > > >
>>> > > > > > > wt., 28 wrz 2021 o 14:49 Leonard Xu <xb...@gmail.com>
>>> > > napisał(a):
>>> > > > > > >
>>> > > > > > >>>>
>>> > > > > > >>>> Not sure if this will happen in 1.15 already. We will
>>> needed
>>> > > > > automated
>>> > > > > > >>>> compatibility tests and a well-defined list of stable API.
>>> > > > > > >>
>>> > > > > > >>> We are
>>> > > > > > >>> trying to provide forward compatibility: applications using
>>> > > > `@Public`
>>> > > > > > >> APIs
>>> > > > > > >>> compiled against Flink 1.12.x, should work fine in Flink
>>> 1.13.x
>>> > > > > > >>
>>> > > > > > >> Unfortunately, I also meet forward compatibility issue,
>>> when I
>>> > do
>>> > > > the
>>> > > > > > >> release 1.14 check, I try to use mysql-cdc connector[1]
>>> which
>>> > > > compiled
>>> > > > > > >> against 1.13.1in SQL Client, but it can not work in flink
>>> 1.14.0
>>> > > > > > cluster,
>>> > > > > > >> it failed due to the metric API compatibility broken.
>>> > > > > > >>
>>> > > > > > >> @Public
>>> > > > > > >> public interface SourceReaderContext {
>>> > > > > > >>
>>> > > > > > >> MetricGroup metricGroup();
>>> > > > > > >>
>>> > > > > > >>
>>> > > > > > >> @Public
>>> > > > > > >> public interface SourceReaderContext {
>>> > > > > > >>
>>> > > > > > >> SourceReaderMetricGroup metricGroup();
>>> > > > > > >>
>>> > > > > > >>
>>> > > > > > >> Shouldn't we mark it as @Deprecated and then delete it util
>>> > 2.0.0
>>> > > > > > version
>>> > > > > > >> for @Public API as the our community rule [2] described? At
>>> > least
>>> > > we
>>> > > > > > should
>>> > > > > > >> keep them across server minor versions
>>> > (<major>.<minor>.<patch>).
>>> > > > > > >>
>>> > > > > > >> Although these changes can be tracked to voted FLIPs and
>>> it’s
>>> > not
>>> > > > the
>>> > > > > > >> fault of a few developers, it show us the fact that we
>>> didn’t
>>> > pay
>>> > > > > enough
>>> > > > > > >> attention to back compatibility/forward compatibility.
>>> > > > > > >>
>>> > > > > > >> Best,
>>> > > > > > >> Leonard
>>> > > > > > >> [1]
>>> > > > > > >>
>>> > > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
>>> > > > > > >> [2]
>>> > > > > > >>
>>> > > > >
>>> > >
>>> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
>>> > > > > > >>
>>> > > > > > >>
>>> > > > > >
>>> > > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>>
>>
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Arvid Heise <ar...@ververica.com>.
The issue is that if we do not mark them as Public, we will always have
incompatibilities. The change of SourceReaderContext#metricGroup is
perfectly fine according to the annotation. The implications that we see
here just mean that the interfaces have been expected to be Public.
And now the question is what do we expect?
Should we expect connector devs to release different connector binaries for
different Flink minors? Then PublicEvolving is fine.
If we expect that the same connector can work across multiple Flink
versions, we need to go into Public.
It doesn't make sense to keep them PublicEvolving on the annotation but
implicitly assume them to be Public.
@Jark Wu <im...@gmail.com> I don't see a way to revert the change of
SourceReaderContext#metricGroup. For now, connector devs that expose
metrics need to release 2 versions. If we change it back, then a specific
connector would work for 1.14.1 and 1.13.X but not for 1.14.0 and this
would be even more confusing.
On Fri, Oct 1, 2021 at 10:49 AM Ingo Bürk <in...@ververica.com> wrote:
> Hi,
>
> > [...] but also the new Source/Sink APIs as public
>
> I'm not really involved in the new Source/Sink APIs and will happily
> listen to the developers working with them here, but since they are new, we
> should also be careful not to mark them as stable too quickly. We've only
> begun updating the existing connectors to these interfaces at the moment.
> Making more progress here and keeping new APIs as Evolving for a couple of
> minor releases is probably still a good idea. Maybe we should even have
> actual rules on when APIs can/should be promoted?
>
> More actively checking backwards-compatibility during a release sounds
> like a great idea regardless, of course.
>
>
> Ingo
>
> On Fri, Oct 1, 2021 at 9:19 AM Jark Wu <im...@gmail.com> wrote:
>
>> Hi all,
>>
>> Nice thread and great discussion! Ecosystem is one of the most important
>> things
>> to the Flink community, we should pay more attention to API compatibility.
>>
>> Marking all connector APIs @Public is a good idea, not only mark the
>> Table/SQL
>> connector APIs public, but also the new Source/Sink APIs as public.
>> Besides, we should also add a check item to the Verify Release
>> documentation[1]
>> to verify the release is backward-compatible for connectors. From my point
>> of view,
>> such backward incompatibility should cancel the vote.
>>
>> Regarding the SourceReaderContext#metricGroup compatibility problem in
>> 1.14.0, I would
>> suggest starting a new discussion thread to see whether we have any idea
>> to
>> fix it. We should
>> fix it ASAP! Otherwise iceberg/hudi/cdc communities will get frustrated
>> again when upgrading
>> to 1.14. Maybe we still have time to release a minor version, because
>> there is no
>> connector upgraded to 1.14.0 yet. What do you think? @Leonard Xu
>> <xb...@gmail.com> @Arvid Heise <ar...@ververica.com> @Piotr Nowojski
>> <pi...@ververica.com>
>>
>> Best,
>> Jark
>>
>> [1]:
>>
>> https://cwiki.apache.org/confluence/display/FLINK/Verifying+a+Flink+Release
>>
>> On Wed, 29 Sept 2021 at 09:46, OpenInx <op...@gmail.com> wrote:
>>
>> > > Apart from this being `@PublicEvolving`
>> >
>> > From my perspective, annotating the 'DynamicTableSink' to be a
>> > 'PublicEvolving' class is not reasonable, because that means devs could
>> > just change the basic API which all downstream connectors are depending
>> on
>> > easily when iterating flink from 1.12 to 1.13 (according to the wiki
>> [1]).
>> > This implies all downstream maintainers must take on this maintenance
>> > burden, and it also makes our flink ecosystem very fragile. Changing
>> the
>> > 'DynamicTableSink' between two major versions sounds reasonable to me,
>> but
>> > unreasonable for uncompatibility changes between two minor versions. I
>> > think we may need to check those API which are annotated
>> 'PublicEnvoling'
>> > while should be 'Public' because of the dependency from all connectors.
>> > We should ensure the stability of those APIs that are necessary to
>> > implement the connector, and at the same time implement the updated v2
>> > version of the API. After all v2 APIs are considered stable, we will
>> mark
>> > them as stable. Instead of releasing a version of the API, some of the
>> APIs
>> > necessary to implement the connector are marked as stable and some are
>> > marked as unstable, which is very unfriendly to downstream. Because
>> > downstream essentially every upgrade requires refactoring of the code.
>> >
>> > > We are trying to provide forward compatibility: applications using
>> > `@Public` APIs compiled against Flink 1.12.x, should work fine in Flink
>> > 1.13.x
>> >
>> > Thanks for clarifying this. Sounds reasonable to me, then we apache
>> > iceberg could just use flink 1.12.x to build the flink+iceberg connector
>> > and should make all the tests work fine for both flink 1.12 & flink
>> 1.13.
>> > For the `ResolvedCatalogTable` changes, I don't think it guarantees the
>> > forward compatibility as you said, because the flink-iceberg-runtime.jar
>> > compiled by flink 1.12 can still not works fine in flink 1.13 because it
>> > will need the flink1.12's `CatalogTable` to be cast to a flink1.13 's
>> > `ResolvedCatalogTable`. In general, I agree that forward
>> compatibility is
>> > a more clear compatibility guarantee.
>> >
>> > [1].
>> > https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
>> >
>> >
>> > On Tue, Sep 28, 2021 at 10:33 PM David Morávek <dm...@apache.org> wrote:
>> >
>> > > >
>> > > > I think we have a compile time checks for breaking changes in
>> `@Public`
>> > > > marked classes/interfaces using japicmp [1].
>> > >
>> > >
>> > > Nice, thanks for pointing that out, I'll take a closer look at it ;)
>> > >
>> > > Best,
>> > > D.
>> > >
>> > > On Tue, Sep 28, 2021 at 4:14 PM Piotr Nowojski <pn...@apache.org>
>> > > wrote:
>> > >
>> > > > > - We don't have any safeguards for stable API breaks. Big +1 for
>> > Ingo's
>> > > > effort with architectural tests [3].
>> > > >
>> > > > I think we have a compile time checks for breaking changes in
>> `@Public`
>> > > > marked classes/interfaces using japicmp [1].
>> > > >
>> > > > Piotrek
>> > > >
>> > > > [1] https://github.com/apache/flink/blob/master/pom.xml#L2014:L2084
>> > > >
>> > > > wt., 28 wrz 2021 o 15:59 David Morávek <dm...@apache.org>
>> napisał(a):
>> > > >
>> > > > > This is a super interesting topic and there is already a great
>> > > > discussion.
>> > > > > Here are few thoughts:
>> > > > >
>> > > > > - There is a delicate balance between fast delivery of the new
>> > features
>> > > > and
>> > > > > API stability. Even though we should be careful with breaking
>> > evolving
>> > > > > interfaces, it shouldn't stop us from making fast progress /
>> iterate
>> > on
>> > > > > features.
>> > > > > - There are two camps of users. One camp prefers more frequent
>> > > releases /
>> > > > > new features (early adopters) and second that prefer less frequent
>> > > stable
>> > > > > releases. There was already a great discussion about this at Flink
>> > 1.14
>> > > > > thread [1].
>> > > > > - We're already trying to be explicit about which API's may break
>> via
>> > > > > annotations and the feature radar [2]. Stability annotations are a
>> > well
>> > > > > known concept used by many projects. I think we still can go bit
>> > > further
>> > > > > here and aim for an IDE support (for example usages of guava
>> > > > @Experimental
>> > > > > interfaces get highlighted, raising more awareness about potential
>> > > > issues).
>> > > > > I'm not sure how this IDE integration works though.
>> > > > > - We don't have any safeguards for stable API breaks. Big +1 for
>> > Ingo's
>> > > > > effort with architectural tests [3].
>> > > > >
>> > > > > [1]
>> > > > >
>> > > > >
>> > > >
>> > >
>> >
>> https://lists.apache.org/thread.html/r76e1cdba577c6f4d6c86b23fdaeb53c4e3744c20d0b3e850fc2e14a7%40%3Cdev.flink.apache.org%3E
>> > > > > [2] https://flink.apache.org/roadmap.html
>> > > > > [3] https://issues.apache.org/jira/browse/FLINK-24138
>> > > > >
>> > > > > Best,
>> > > > > D.
>> > > > >
>> > > > > On Tue, Sep 28, 2021 at 3:49 PM Leonard Xu <xb...@gmail.com>
>> > wrote:
>> > > > >
>> > > > > > Thanks Piotr for the kindly reply, what confused me is that
>> > > > > > `SourceReaderContext` was marked @Public when it was born in
>> flink
>> > > > 1.11,
>> > > > > > and then it was corrected to @PublicEvolving in 1.11 -_-, and
>> > finally
>> > > > it
>> > > > > > was changed to @Public again...
>> > > > > >
>> > > > > > As Flink and Flink ecosystem(Flink CDC connectors) developer, I
>> > think
>> > > > > what
>> > > > > > we're discussing is meaningful and I’d like to help improve
>> those
>> > > > Public
>> > > > > > API check for those changes.
>> > > > > > Chesnay’s tips is a good idea that maybe we can use the tool
>> like
>> > > > japicmp
>> > > > > > to do the check for every PR in CI phase.
>> > > > > >
>> > > > > > Best,
>> > > > > > Leonard
>> > > > > >
>> > > > > >
>> > > > > > > 在 2021年9月28日，21:15，Piotr Nowojski <pn...@apache.org> 写道：
>> > > > > > >
>> > > > > > > Hi Leonard,
>> > > > > > >
>> > > > > > > Sorry for this causing you troubles, however that change in
>> the
>> > > > return
>> > > > > > type
>> > > > > > > was done while this class still has been marked as
>> > > > > `@PublicEvolving`[1].
>> > > > > > As
>> > > > > > > of 1.13.x `SourceReaderContext` was `@PublicEvolving` and it
>> was
>> > > > marked
>> > > > > > as
>> > > > > > > `@Public` only starting from Flink 1.14.0 [2]. Probably what
>> > > confused
>> > > > > you
>> > > > > > > was that both of those changes (changing the return type and
>> > making
>> > > > it
>> > > > > > > `@Public`) happened in the same release.
>> > > > > > >
>> > > > > > > However, those changes (`SourceReaderContext` and
>> > > > > `ResolvedCatalogTable`)
>> > > > > > > should have been clearly mentioned in the release notes with
>> an
>> > > > upgrade
>> > > > > > > guide.
>> > > > > > >
>> > > > > > > Best, Piotrek
>> > > > > > >
>> > > > > > > [1]
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>> https://github.com/apache/flink/commit/7f3636f6b4f8bac415a7db85917ad849636bd730#diff-a247a24ccd1afc07c5d690a8a58b1f6584329925fdf0d7dc89361b90d621b7f2R31
>> > > > > > > [2] https://issues.apache.org/jira/browse/FLINK-22357
>> > > > > > >
>> > > > > > > wt., 28 wrz 2021 o 14:49 Leonard Xu <xb...@gmail.com>
>> > > napisał(a):
>> > > > > > >
>> > > > > > >>>>
>> > > > > > >>>> Not sure if this will happen in 1.15 already. We will
>> needed
>> > > > > automated
>> > > > > > >>>> compatibility tests and a well-defined list of stable API.
>> > > > > > >>
>> > > > > > >>> We are
>> > > > > > >>> trying to provide forward compatibility: applications using
>> > > > `@Public`
>> > > > > > >> APIs
>> > > > > > >>> compiled against Flink 1.12.x, should work fine in Flink
>> 1.13.x
>> > > > > > >>
>> > > > > > >> Unfortunately, I also meet forward compatibility issue, when
>> I
>> > do
>> > > > the
>> > > > > > >> release 1.14 check, I try to use mysql-cdc connector[1] which
>> > > > compiled
>> > > > > > >> against 1.13.1in SQL Client, but it can not work in flink
>> 1.14.0
>> > > > > > cluster,
>> > > > > > >> it failed due to the metric API compatibility broken.
>> > > > > > >>
>> > > > > > >> @Public
>> > > > > > >> public interface SourceReaderContext {
>> > > > > > >>
>> > > > > > >> MetricGroup metricGroup();
>> > > > > > >>
>> > > > > > >>
>> > > > > > >> @Public
>> > > > > > >> public interface SourceReaderContext {
>> > > > > > >>
>> > > > > > >> SourceReaderMetricGroup metricGroup();
>> > > > > > >>
>> > > > > > >>
>> > > > > > >> Shouldn't we mark it as @Deprecated and then delete it util
>> > 2.0.0
>> > > > > > version
>> > > > > > >> for @Public API as the our community rule [2] described? At
>> > least
>> > > we
>> > > > > > should
>> > > > > > >> keep them across server minor versions
>> > (<major>.<minor>.<patch>).
>> > > > > > >>
>> > > > > > >> Although these changes can be tracked to voted FLIPs and it’s
>> > not
>> > > > the
>> > > > > > >> fault of a few developers, it show us the fact that we didn’t
>> > pay
>> > > > > enough
>> > > > > > >> attention to back compatibility/forward compatibility.
>> > > > > > >>
>> > > > > > >> Best,
>> > > > > > >> Leonard
>> > > > > > >> [1]
>> > > > > > >>
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
>> > > > > > >> [2]
>> > > > > > >>
>> > > > >
>> > >
>> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
>> > > > > > >>
>> > > > > > >>
>> > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Ingo Bürk <in...@ververica.com>.
Hi,
> [...] but also the new Source/Sink APIs as public
I'm not really involved in the new Source/Sink APIs and will happily listen
to the developers working with them here, but since they are new, we should
also be careful not to mark them as stable too quickly. We've only begun
updating the existing connectors to these interfaces at the moment. Making
more progress here and keeping new APIs as Evolving for a couple of minor
releases is probably still a good idea. Maybe we should even have actual
rules on when APIs can/should be promoted?
More actively checking backwards-compatibility during a release sounds like
a great idea regardless, of course.
Ingo
On Fri, Oct 1, 2021 at 9:19 AM Jark Wu <im...@gmail.com> wrote:
> Hi all,
>
> Nice thread and great discussion! Ecosystem is one of the most important
> things
> to the Flink community, we should pay more attention to API compatibility.
>
> Marking all connector APIs @Public is a good idea, not only mark the
> Table/SQL
> connector APIs public, but also the new Source/Sink APIs as public.
> Besides, we should also add a check item to the Verify Release
> documentation[1]
> to verify the release is backward-compatible for connectors. From my point
> of view,
> such backward incompatibility should cancel the vote.
>
> Regarding the SourceReaderContext#metricGroup compatibility problem in
> 1.14.0, I would
> suggest starting a new discussion thread to see whether we have any idea to
> fix it. We should
> fix it ASAP! Otherwise iceberg/hudi/cdc communities will get frustrated
> again when upgrading
> to 1.14. Maybe we still have time to release a minor version, because
> there is no
> connector upgraded to 1.14.0 yet. What do you think? @Leonard Xu
> <xb...@gmail.com> @Arvid Heise <ar...@ververica.com> @Piotr Nowojski
> <pi...@ververica.com>
>
> Best,
> Jark
>
> [1]:
> https://cwiki.apache.org/confluence/display/FLINK/Verifying+a+Flink+Release
>
> On Wed, 29 Sept 2021 at 09:46, OpenInx <op...@gmail.com> wrote:
>
> > > Apart from this being `@PublicEvolving`
> >
> > From my perspective, annotating the 'DynamicTableSink' to be a
> > 'PublicEvolving' class is not reasonable, because that means devs could
> > just change the basic API which all downstream connectors are depending
> on
> > easily when iterating flink from 1.12 to 1.13 (according to the wiki
> [1]).
> > This implies all downstream maintainers must take on this maintenance
> > burden, and it also makes our flink ecosystem very fragile. Changing
> the
> > 'DynamicTableSink' between two major versions sounds reasonable to me,
> but
> > unreasonable for uncompatibility changes between two minor versions. I
> > think we may need to check those API which are annotated 'PublicEnvoling'
> > while should be 'Public' because of the dependency from all connectors.
> > We should ensure the stability of those APIs that are necessary to
> > implement the connector, and at the same time implement the updated v2
> > version of the API. After all v2 APIs are considered stable, we will mark
> > them as stable. Instead of releasing a version of the API, some of the
> APIs
> > necessary to implement the connector are marked as stable and some are
> > marked as unstable, which is very unfriendly to downstream. Because
> > downstream essentially every upgrade requires refactoring of the code.
> >
> > > We are trying to provide forward compatibility: applications using
> > `@Public` APIs compiled against Flink 1.12.x, should work fine in Flink
> > 1.13.x
> >
> > Thanks for clarifying this. Sounds reasonable to me, then we apache
> > iceberg could just use flink 1.12.x to build the flink+iceberg connector
> > and should make all the tests work fine for both flink 1.12 & flink 1.13.
> > For the `ResolvedCatalogTable` changes, I don't think it guarantees the
> > forward compatibility as you said, because the flink-iceberg-runtime.jar
> > compiled by flink 1.12 can still not works fine in flink 1.13 because it
> > will need the flink1.12's `CatalogTable` to be cast to a flink1.13 's
> > `ResolvedCatalogTable`. In general, I agree that forward compatibility
> is
> > a more clear compatibility guarantee.
> >
> > [1].
> > https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> >
> >
> > On Tue, Sep 28, 2021 at 10:33 PM David Morávek <dm...@apache.org> wrote:
> >
> > > >
> > > > I think we have a compile time checks for breaking changes in
> `@Public`
> > > > marked classes/interfaces using japicmp [1].
> > >
> > >
> > > Nice, thanks for pointing that out, I'll take a closer look at it ;)
> > >
> > > Best,
> > > D.
> > >
> > > On Tue, Sep 28, 2021 at 4:14 PM Piotr Nowojski <pn...@apache.org>
> > > wrote:
> > >
> > > > > - We don't have any safeguards for stable API breaks. Big +1 for
> > Ingo's
> > > > effort with architectural tests [3].
> > > >
> > > > I think we have a compile time checks for breaking changes in
> `@Public`
> > > > marked classes/interfaces using japicmp [1].
> > > >
> > > > Piotrek
> > > >
> > > > [1] https://github.com/apache/flink/blob/master/pom.xml#L2014:L2084
> > > >
> > > > wt., 28 wrz 2021 o 15:59 David Morávek <dm...@apache.org> napisał(a):
> > > >
> > > > > This is a super interesting topic and there is already a great
> > > > discussion.
> > > > > Here are few thoughts:
> > > > >
> > > > > - There is a delicate balance between fast delivery of the new
> > features
> > > > and
> > > > > API stability. Even though we should be careful with breaking
> > evolving
> > > > > interfaces, it shouldn't stop us from making fast progress /
> iterate
> > on
> > > > > features.
> > > > > - There are two camps of users. One camp prefers more frequent
> > > releases /
> > > > > new features (early adopters) and second that prefer less frequent
> > > stable
> > > > > releases. There was already a great discussion about this at Flink
> > 1.14
> > > > > thread [1].
> > > > > - We're already trying to be explicit about which API's may break
> via
> > > > > annotations and the feature radar [2]. Stability annotations are a
> > well
> > > > > known concept used by many projects. I think we still can go bit
> > > further
> > > > > here and aim for an IDE support (for example usages of guava
> > > > @Experimental
> > > > > interfaces get highlighted, raising more awareness about potential
> > > > issues).
> > > > > I'm not sure how this IDE integration works though.
> > > > > - We don't have any safeguards for stable API breaks. Big +1 for
> > Ingo's
> > > > > effort with architectural tests [3].
> > > > >
> > > > > [1]
> > > > >
> > > > >
> > > >
> > >
> >
> https://lists.apache.org/thread.html/r76e1cdba577c6f4d6c86b23fdaeb53c4e3744c20d0b3e850fc2e14a7%40%3Cdev.flink.apache.org%3E
> > > > > [2] https://flink.apache.org/roadmap.html
> > > > > [3] https://issues.apache.org/jira/browse/FLINK-24138
> > > > >
> > > > > Best,
> > > > > D.
> > > > >
> > > > > On Tue, Sep 28, 2021 at 3:49 PM Leonard Xu <xb...@gmail.com>
> > wrote:
> > > > >
> > > > > > Thanks Piotr for the kindly reply, what confused me is that
> > > > > > `SourceReaderContext` was marked @Public when it was born in
> flink
> > > > 1.11,
> > > > > > and then it was corrected to @PublicEvolving in 1.11 -_-, and
> > finally
> > > > it
> > > > > > was changed to @Public again...
> > > > > >
> > > > > > As Flink and Flink ecosystem(Flink CDC connectors) developer, I
> > think
> > > > > what
> > > > > > we're discussing is meaningful and I’d like to help improve those
> > > > Public
> > > > > > API check for those changes.
> > > > > > Chesnay’s tips is a good idea that maybe we can use the tool like
> > > > japicmp
> > > > > > to do the check for every PR in CI phase.
> > > > > >
> > > > > > Best,
> > > > > > Leonard
> > > > > >
> > > > > >
> > > > > > > 在 2021年9月28日，21:15，Piotr Nowojski <pn...@apache.org> 写道：
> > > > > > >
> > > > > > > Hi Leonard,
> > > > > > >
> > > > > > > Sorry for this causing you troubles, however that change in the
> > > > return
> > > > > > type
> > > > > > > was done while this class still has been marked as
> > > > > `@PublicEvolving`[1].
> > > > > > As
> > > > > > > of 1.13.x `SourceReaderContext` was `@PublicEvolving` and it
> was
> > > > marked
> > > > > > as
> > > > > > > `@Public` only starting from Flink 1.14.0 [2]. Probably what
> > > confused
> > > > > you
> > > > > > > was that both of those changes (changing the return type and
> > making
> > > > it
> > > > > > > `@Public`) happened in the same release.
> > > > > > >
> > > > > > > However, those changes (`SourceReaderContext` and
> > > > > `ResolvedCatalogTable`)
> > > > > > > should have been clearly mentioned in the release notes with an
> > > > upgrade
> > > > > > > guide.
> > > > > > >
> > > > > > > Best, Piotrek
> > > > > > >
> > > > > > > [1]
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://github.com/apache/flink/commit/7f3636f6b4f8bac415a7db85917ad849636bd730#diff-a247a24ccd1afc07c5d690a8a58b1f6584329925fdf0d7dc89361b90d621b7f2R31
> > > > > > > [2] https://issues.apache.org/jira/browse/FLINK-22357
> > > > > > >
> > > > > > > wt., 28 wrz 2021 o 14:49 Leonard Xu <xb...@gmail.com>
> > > napisał(a):
> > > > > > >
> > > > > > >>>>
> > > > > > >>>> Not sure if this will happen in 1.15 already. We will needed
> > > > > automated
> > > > > > >>>> compatibility tests and a well-defined list of stable API.
> > > > > > >>
> > > > > > >>> We are
> > > > > > >>> trying to provide forward compatibility: applications using
> > > > `@Public`
> > > > > > >> APIs
> > > > > > >>> compiled against Flink 1.12.x, should work fine in Flink
> 1.13.x
> > > > > > >>
> > > > > > >> Unfortunately, I also meet forward compatibility issue, when I
> > do
> > > > the
> > > > > > >> release 1.14 check, I try to use mysql-cdc connector[1] which
> > > > compiled
> > > > > > >> against 1.13.1in SQL Client, but it can not work in flink
> 1.14.0
> > > > > > cluster,
> > > > > > >> it failed due to the metric API compatibility broken.
> > > > > > >>
> > > > > > >> @Public
> > > > > > >> public interface SourceReaderContext {
> > > > > > >>
> > > > > > >> MetricGroup metricGroup();
> > > > > > >>
> > > > > > >>
> > > > > > >> @Public
> > > > > > >> public interface SourceReaderContext {
> > > > > > >>
> > > > > > >> SourceReaderMetricGroup metricGroup();
> > > > > > >>
> > > > > > >>
> > > > > > >> Shouldn't we mark it as @Deprecated and then delete it util
> > 2.0.0
> > > > > > version
> > > > > > >> for @Public API as the our community rule [2] described? At
> > least
> > > we
> > > > > > should
> > > > > > >> keep them across server minor versions
> > (<major>.<minor>.<patch>).
> > > > > > >>
> > > > > > >> Although these changes can be tracked to voted FLIPs and it’s
> > not
> > > > the
> > > > > > >> fault of a few developers, it show us the fact that we didn’t
> > pay
> > > > > enough
> > > > > > >> attention to back compatibility/forward compatibility.
> > > > > > >>
> > > > > > >> Best,
> > > > > > >> Leonard
> > > > > > >> [1]
> > > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> >
> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
> > > > > > >> [2]
> > > > > > >>
> > > > >
> > >
> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> > > > > > >>
> > > > > > >>
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Jark Wu <im...@gmail.com>.
Hi all,
Nice thread and great discussion! Ecosystem is one of the most important
things
to the Flink community, we should pay more attention to API compatibility.
Marking all connector APIs @Public is a good idea, not only mark the
Table/SQL
connector APIs public, but also the new Source/Sink APIs as public.
Besides, we should also add a check item to the Verify Release
documentation[1]
to verify the release is backward-compatible for connectors. From my point
of view,
such backward incompatibility should cancel the vote.
Regarding the SourceReaderContext#metricGroup compatibility problem in
1.14.0, I would
suggest starting a new discussion thread to see whether we have any idea to
fix it. We should
fix it ASAP! Otherwise iceberg/hudi/cdc communities will get frustrated
again when upgrading
to 1.14. Maybe we still have time to release a minor version, because
there is no
connector upgraded to 1.14.0 yet. What do you think? @Leonard Xu
<xb...@gmail.com> @Arvid Heise <ar...@ververica.com> @Piotr Nowojski
<pi...@ververica.com>
Best,
Jark
[1]:
https://cwiki.apache.org/confluence/display/FLINK/Verifying+a+Flink+Release
On Wed, 29 Sept 2021 at 09:46, OpenInx <op...@gmail.com> wrote:
> > Apart from this being `@PublicEvolving`
>
> From my perspective, annotating the 'DynamicTableSink' to be a
> 'PublicEvolving' class is not reasonable, because that means devs could
> just change the basic API which all downstream connectors are depending on
> easily when iterating flink from 1.12 to 1.13 (according to the wiki [1]).
> This implies all downstream maintainers must take on this maintenance
> burden, and it also makes our flink ecosystem very fragile. Changing the
> 'DynamicTableSink' between two major versions sounds reasonable to me, but
> unreasonable for uncompatibility changes between two minor versions. I
> think we may need to check those API which are annotated 'PublicEnvoling'
> while should be 'Public' because of the dependency from all connectors.
> We should ensure the stability of those APIs that are necessary to
> implement the connector, and at the same time implement the updated v2
> version of the API. After all v2 APIs are considered stable, we will mark
> them as stable. Instead of releasing a version of the API, some of the APIs
> necessary to implement the connector are marked as stable and some are
> marked as unstable, which is very unfriendly to downstream. Because
> downstream essentially every upgrade requires refactoring of the code.
>
> > We are trying to provide forward compatibility: applications using
> `@Public` APIs compiled against Flink 1.12.x, should work fine in Flink
> 1.13.x
>
> Thanks for clarifying this. Sounds reasonable to me, then we apache
> iceberg could just use flink 1.12.x to build the flink+iceberg connector
> and should make all the tests work fine for both flink 1.12 & flink 1.13.
> For the `ResolvedCatalogTable` changes, I don't think it guarantees the
> forward compatibility as you said, because the flink-iceberg-runtime.jar
> compiled by flink 1.12 can still not works fine in flink 1.13 because it
> will need the flink1.12's `CatalogTable` to be cast to a flink1.13 's
> `ResolvedCatalogTable`. In general, I agree that forward compatibility is
> a more clear compatibility guarantee.
>
> [1].
> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
>
>
> On Tue, Sep 28, 2021 at 10:33 PM David Morávek <dm...@apache.org> wrote:
>
> > >
> > > I think we have a compile time checks for breaking changes in `@Public`
> > > marked classes/interfaces using japicmp [1].
> >
> >
> > Nice, thanks for pointing that out, I'll take a closer look at it ;)
> >
> > Best,
> > D.
> >
> > On Tue, Sep 28, 2021 at 4:14 PM Piotr Nowojski <pn...@apache.org>
> > wrote:
> >
> > > > - We don't have any safeguards for stable API breaks. Big +1 for
> Ingo's
> > > effort with architectural tests [3].
> > >
> > > I think we have a compile time checks for breaking changes in `@Public`
> > > marked classes/interfaces using japicmp [1].
> > >
> > > Piotrek
> > >
> > > [1] https://github.com/apache/flink/blob/master/pom.xml#L2014:L2084
> > >
> > > wt., 28 wrz 2021 o 15:59 David Morávek <dm...@apache.org> napisał(a):
> > >
> > > > This is a super interesting topic and there is already a great
> > > discussion.
> > > > Here are few thoughts:
> > > >
> > > > - There is a delicate balance between fast delivery of the new
> features
> > > and
> > > > API stability. Even though we should be careful with breaking
> evolving
> > > > interfaces, it shouldn't stop us from making fast progress / iterate
> on
> > > > features.
> > > > - There are two camps of users. One camp prefers more frequent
> > releases /
> > > > new features (early adopters) and second that prefer less frequent
> > stable
> > > > releases. There was already a great discussion about this at Flink
> 1.14
> > > > thread [1].
> > > > - We're already trying to be explicit about which API's may break via
> > > > annotations and the feature radar [2]. Stability annotations are a
> well
> > > > known concept used by many projects. I think we still can go bit
> > further
> > > > here and aim for an IDE support (for example usages of guava
> > > @Experimental
> > > > interfaces get highlighted, raising more awareness about potential
> > > issues).
> > > > I'm not sure how this IDE integration works though.
> > > > - We don't have any safeguards for stable API breaks. Big +1 for
> Ingo's
> > > > effort with architectural tests [3].
> > > >
> > > > [1]
> > > >
> > > >
> > >
> >
> https://lists.apache.org/thread.html/r76e1cdba577c6f4d6c86b23fdaeb53c4e3744c20d0b3e850fc2e14a7%40%3Cdev.flink.apache.org%3E
> > > > [2] https://flink.apache.org/roadmap.html
> > > > [3] https://issues.apache.org/jira/browse/FLINK-24138
> > > >
> > > > Best,
> > > > D.
> > > >
> > > > On Tue, Sep 28, 2021 at 3:49 PM Leonard Xu <xb...@gmail.com>
> wrote:
> > > >
> > > > > Thanks Piotr for the kindly reply, what confused me is that
> > > > > `SourceReaderContext` was marked @Public when it was born in flink
> > > 1.11,
> > > > > and then it was corrected to @PublicEvolving in 1.11 -_-, and
> finally
> > > it
> > > > > was changed to @Public again...
> > > > >
> > > > > As Flink and Flink ecosystem(Flink CDC connectors) developer, I
> think
> > > > what
> > > > > we're discussing is meaningful and I’d like to help improve those
> > > Public
> > > > > API check for those changes.
> > > > > Chesnay’s tips is a good idea that maybe we can use the tool like
> > > japicmp
> > > > > to do the check for every PR in CI phase.
> > > > >
> > > > > Best,
> > > > > Leonard
> > > > >
> > > > >
> > > > > > 在 2021年9月28日，21:15，Piotr Nowojski <pn...@apache.org> 写道：
> > > > > >
> > > > > > Hi Leonard,
> > > > > >
> > > > > > Sorry for this causing you troubles, however that change in the
> > > return
> > > > > type
> > > > > > was done while this class still has been marked as
> > > > `@PublicEvolving`[1].
> > > > > As
> > > > > > of 1.13.x `SourceReaderContext` was `@PublicEvolving` and it was
> > > marked
> > > > > as
> > > > > > `@Public` only starting from Flink 1.14.0 [2]. Probably what
> > confused
> > > > you
> > > > > > was that both of those changes (changing the return type and
> making
> > > it
> > > > > > `@Public`) happened in the same release.
> > > > > >
> > > > > > However, those changes (`SourceReaderContext` and
> > > > `ResolvedCatalogTable`)
> > > > > > should have been clearly mentioned in the release notes with an
> > > upgrade
> > > > > > guide.
> > > > > >
> > > > > > Best, Piotrek
> > > > > >
> > > > > > [1]
> > > > > >
> > > > >
> > > >
> > >
> >
> https://github.com/apache/flink/commit/7f3636f6b4f8bac415a7db85917ad849636bd730#diff-a247a24ccd1afc07c5d690a8a58b1f6584329925fdf0d7dc89361b90d621b7f2R31
> > > > > > [2] https://issues.apache.org/jira/browse/FLINK-22357
> > > > > >
> > > > > > wt., 28 wrz 2021 o 14:49 Leonard Xu <xb...@gmail.com>
> > napisał(a):
> > > > > >
> > > > > >>>>
> > > > > >>>> Not sure if this will happen in 1.15 already. We will needed
> > > > automated
> > > > > >>>> compatibility tests and a well-defined list of stable API.
> > > > > >>
> > > > > >>> We are
> > > > > >>> trying to provide forward compatibility: applications using
> > > `@Public`
> > > > > >> APIs
> > > > > >>> compiled against Flink 1.12.x, should work fine in Flink 1.13.x
> > > > > >>
> > > > > >> Unfortunately, I also meet forward compatibility issue, when I
> do
> > > the
> > > > > >> release 1.14 check, I try to use mysql-cdc connector[1] which
> > > compiled
> > > > > >> against 1.13.1in SQL Client, but it can not work in flink 1.14.0
> > > > > cluster,
> > > > > >> it failed due to the metric API compatibility broken.
> > > > > >>
> > > > > >> @Public
> > > > > >> public interface SourceReaderContext {
> > > > > >>
> > > > > >> MetricGroup metricGroup();
> > > > > >>
> > > > > >>
> > > > > >> @Public
> > > > > >> public interface SourceReaderContext {
> > > > > >>
> > > > > >> SourceReaderMetricGroup metricGroup();
> > > > > >>
> > > > > >>
> > > > > >> Shouldn't we mark it as @Deprecated and then delete it util
> 2.0.0
> > > > > version
> > > > > >> for @Public API as the our community rule [2] described? At
> least
> > we
> > > > > should
> > > > > >> keep them across server minor versions
> (<major>.<minor>.<patch>).
> > > > > >>
> > > > > >> Although these changes can be tracked to voted FLIPs and it’s
> not
> > > the
> > > > > >> fault of a few developers, it show us the fact that we didn’t
> pay
> > > > enough
> > > > > >> attention to back compatibility/forward compatibility.
> > > > > >>
> > > > > >> Best,
> > > > > >> Leonard
> > > > > >> [1]
> > > > > >>
> > > > >
> > > >
> > >
> >
> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
> > > > > >> [2]
> > > > > >>
> > > >
> > https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> > > > > >>
> > > > > >>
> > > > >
> > > > >
> > > >
> > >
> >
>
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by OpenInx <op...@gmail.com>.
> Apart from this being `@PublicEvolving`
From my perspective, annotating the 'DynamicTableSink' to be a
'PublicEvolving' class is not reasonable, because that means devs could
just change the basic API which all downstream connectors are depending on
easily when iterating flink from 1.12 to 1.13 (according to the wiki [1]).
This implies all downstream maintainers must take on this maintenance
burden, and it also makes our flink ecosystem very fragile. Changing the
'DynamicTableSink' between two major versions sounds reasonable to me, but
unreasonable for uncompatibility changes between two minor versions. I
think we may need to check those API which are annotated 'PublicEnvoling'
while should be 'Public' because of the dependency from all connectors.
We should ensure the stability of those APIs that are necessary to
implement the connector, and at the same time implement the updated v2
version of the API. After all v2 APIs are considered stable, we will mark
them as stable. Instead of releasing a version of the API, some of the APIs
necessary to implement the connector are marked as stable and some are
marked as unstable, which is very unfriendly to downstream. Because
downstream essentially every upgrade requires refactoring of the code.
> We are trying to provide forward compatibility: applications using
`@Public` APIs compiled against Flink 1.12.x, should work fine in Flink
1.13.x
Thanks for clarifying this. Sounds reasonable to me, then we apache
iceberg could just use flink 1.12.x to build the flink+iceberg connector
and should make all the tests work fine for both flink 1.12 & flink 1.13.
For the `ResolvedCatalogTable` changes, I don't think it guarantees the
forward compatibility as you said, because the flink-iceberg-runtime.jar
compiled by flink 1.12 can still not works fine in flink 1.13 because it
will need the flink1.12's `CatalogTable` to be cast to a flink1.13 's
`ResolvedCatalogTable`. In general, I agree that forward compatibility is
a more clear compatibility guarantee.
[1]. https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
On Tue, Sep 28, 2021 at 10:33 PM David Morávek <dm...@apache.org> wrote:
> >
> > I think we have a compile time checks for breaking changes in `@Public`
> > marked classes/interfaces using japicmp [1].
>
>
> Nice, thanks for pointing that out, I'll take a closer look at it ;)
>
> Best,
> D.
>
> On Tue, Sep 28, 2021 at 4:14 PM Piotr Nowojski <pn...@apache.org>
> wrote:
>
> > > - We don't have any safeguards for stable API breaks. Big +1 for Ingo's
> > effort with architectural tests [3].
> >
> > I think we have a compile time checks for breaking changes in `@Public`
> > marked classes/interfaces using japicmp [1].
> >
> > Piotrek
> >
> > [1] https://github.com/apache/flink/blob/master/pom.xml#L2014:L2084
> >
> > wt., 28 wrz 2021 o 15:59 David Morávek <dm...@apache.org> napisał(a):
> >
> > > This is a super interesting topic and there is already a great
> > discussion.
> > > Here are few thoughts:
> > >
> > > - There is a delicate balance between fast delivery of the new features
> > and
> > > API stability. Even though we should be careful with breaking evolving
> > > interfaces, it shouldn't stop us from making fast progress / iterate on
> > > features.
> > > - There are two camps of users. One camp prefers more frequent
> releases /
> > > new features (early adopters) and second that prefer less frequent
> stable
> > > releases. There was already a great discussion about this at Flink 1.14
> > > thread [1].
> > > - We're already trying to be explicit about which API's may break via
> > > annotations and the feature radar [2]. Stability annotations are a well
> > > known concept used by many projects. I think we still can go bit
> further
> > > here and aim for an IDE support (for example usages of guava
> > @Experimental
> > > interfaces get highlighted, raising more awareness about potential
> > issues).
> > > I'm not sure how this IDE integration works though.
> > > - We don't have any safeguards for stable API breaks. Big +1 for Ingo's
> > > effort with architectural tests [3].
> > >
> > > [1]
> > >
> > >
> >
> https://lists.apache.org/thread.html/r76e1cdba577c6f4d6c86b23fdaeb53c4e3744c20d0b3e850fc2e14a7%40%3Cdev.flink.apache.org%3E
> > > [2] https://flink.apache.org/roadmap.html
> > > [3] https://issues.apache.org/jira/browse/FLINK-24138
> > >
> > > Best,
> > > D.
> > >
> > > On Tue, Sep 28, 2021 at 3:49 PM Leonard Xu <xb...@gmail.com> wrote:
> > >
> > > > Thanks Piotr for the kindly reply, what confused me is that
> > > > `SourceReaderContext` was marked @Public when it was born in flink
> > 1.11,
> > > > and then it was corrected to @PublicEvolving in 1.11 -_-, and finally
> > it
> > > > was changed to @Public again...
> > > >
> > > > As Flink and Flink ecosystem(Flink CDC connectors) developer, I think
> > > what
> > > > we're discussing is meaningful and I’d like to help improve those
> > Public
> > > > API check for those changes.
> > > > Chesnay’s tips is a good idea that maybe we can use the tool like
> > japicmp
> > > > to do the check for every PR in CI phase.
> > > >
> > > > Best,
> > > > Leonard
> > > >
> > > >
> > > > > 在 2021年9月28日，21:15，Piotr Nowojski <pn...@apache.org> 写道：
> > > > >
> > > > > Hi Leonard,
> > > > >
> > > > > Sorry for this causing you troubles, however that change in the
> > return
> > > > type
> > > > > was done while this class still has been marked as
> > > `@PublicEvolving`[1].
> > > > As
> > > > > of 1.13.x `SourceReaderContext` was `@PublicEvolving` and it was
> > marked
> > > > as
> > > > > `@Public` only starting from Flink 1.14.0 [2]. Probably what
> confused
> > > you
> > > > > was that both of those changes (changing the return type and making
> > it
> > > > > `@Public`) happened in the same release.
> > > > >
> > > > > However, those changes (`SourceReaderContext` and
> > > `ResolvedCatalogTable`)
> > > > > should have been clearly mentioned in the release notes with an
> > upgrade
> > > > > guide.
> > > > >
> > > > > Best, Piotrek
> > > > >
> > > > > [1]
> > > > >
> > > >
> > >
> >
> https://github.com/apache/flink/commit/7f3636f6b4f8bac415a7db85917ad849636bd730#diff-a247a24ccd1afc07c5d690a8a58b1f6584329925fdf0d7dc89361b90d621b7f2R31
> > > > > [2] https://issues.apache.org/jira/browse/FLINK-22357
> > > > >
> > > > > wt., 28 wrz 2021 o 14:49 Leonard Xu <xb...@gmail.com>
> napisał(a):
> > > > >
> > > > >>>>
> > > > >>>> Not sure if this will happen in 1.15 already. We will needed
> > > automated
> > > > >>>> compatibility tests and a well-defined list of stable API.
> > > > >>
> > > > >>> We are
> > > > >>> trying to provide forward compatibility: applications using
> > `@Public`
> > > > >> APIs
> > > > >>> compiled against Flink 1.12.x, should work fine in Flink 1.13.x
> > > > >>
> > > > >> Unfortunately, I also meet forward compatibility issue, when I do
> > the
> > > > >> release 1.14 check, I try to use mysql-cdc connector[1] which
> > compiled
> > > > >> against 1.13.1in SQL Client, but it can not work in flink 1.14.0
> > > > cluster,
> > > > >> it failed due to the metric API compatibility broken.
> > > > >>
> > > > >> @Public
> > > > >> public interface SourceReaderContext {
> > > > >>
> > > > >> MetricGroup metricGroup();
> > > > >>
> > > > >>
> > > > >> @Public
> > > > >> public interface SourceReaderContext {
> > > > >>
> > > > >> SourceReaderMetricGroup metricGroup();
> > > > >>
> > > > >>
> > > > >> Shouldn't we mark it as @Deprecated and then delete it util 2.0.0
> > > > version
> > > > >> for @Public API as the our community rule [2] described? At least
> we
> > > > should
> > > > >> keep them across server minor versions (<major>.<minor>.<patch>).
> > > > >>
> > > > >> Although these changes can be tracked to voted FLIPs and it’s not
> > the
> > > > >> fault of a few developers, it show us the fact that we didn’t pay
> > > enough
> > > > >> attention to back compatibility/forward compatibility.
> > > > >>
> > > > >> Best,
> > > > >> Leonard
> > > > >> [1]
> > > > >>
> > > >
> > >
> >
> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
> > > > >> [2]
> > > > >>
> > >
> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> > > > >>
> > > > >>
> > > >
> > > >
> > >
> >
>
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by David Morávek <dm...@apache.org>.
>
> I think we have a compile time checks for breaking changes in `@Public`
> marked classes/interfaces using japicmp [1].
Nice, thanks for pointing that out, I'll take a closer look at it ;)
Best,
D.
On Tue, Sep 28, 2021 at 4:14 PM Piotr Nowojski <pn...@apache.org> wrote:
> > - We don't have any safeguards for stable API breaks. Big +1 for Ingo's
> effort with architectural tests [3].
>
> I think we have a compile time checks for breaking changes in `@Public`
> marked classes/interfaces using japicmp [1].
>
> Piotrek
>
> [1] https://github.com/apache/flink/blob/master/pom.xml#L2014:L2084
>
> wt., 28 wrz 2021 o 15:59 David Morávek <dm...@apache.org> napisał(a):
>
> > This is a super interesting topic and there is already a great
> discussion.
> > Here are few thoughts:
> >
> > - There is a delicate balance between fast delivery of the new features
> and
> > API stability. Even though we should be careful with breaking evolving
> > interfaces, it shouldn't stop us from making fast progress / iterate on
> > features.
> > - There are two camps of users. One camp prefers more frequent releases /
> > new features (early adopters) and second that prefer less frequent stable
> > releases. There was already a great discussion about this at Flink 1.14
> > thread [1].
> > - We're already trying to be explicit about which API's may break via
> > annotations and the feature radar [2]. Stability annotations are a well
> > known concept used by many projects. I think we still can go bit further
> > here and aim for an IDE support (for example usages of guava
> @Experimental
> > interfaces get highlighted, raising more awareness about potential
> issues).
> > I'm not sure how this IDE integration works though.
> > - We don't have any safeguards for stable API breaks. Big +1 for Ingo's
> > effort with architectural tests [3].
> >
> > [1]
> >
> >
> https://lists.apache.org/thread.html/r76e1cdba577c6f4d6c86b23fdaeb53c4e3744c20d0b3e850fc2e14a7%40%3Cdev.flink.apache.org%3E
> > [2] https://flink.apache.org/roadmap.html
> > [3] https://issues.apache.org/jira/browse/FLINK-24138
> >
> > Best,
> > D.
> >
> > On Tue, Sep 28, 2021 at 3:49 PM Leonard Xu <xb...@gmail.com> wrote:
> >
> > > Thanks Piotr for the kindly reply, what confused me is that
> > > `SourceReaderContext` was marked @Public when it was born in flink
> 1.11,
> > > and then it was corrected to @PublicEvolving in 1.11 -_-, and finally
> it
> > > was changed to @Public again...
> > >
> > > As Flink and Flink ecosystem(Flink CDC connectors) developer, I think
> > what
> > > we're discussing is meaningful and I’d like to help improve those
> Public
> > > API check for those changes.
> > > Chesnay’s tips is a good idea that maybe we can use the tool like
> japicmp
> > > to do the check for every PR in CI phase.
> > >
> > > Best,
> > > Leonard
> > >
> > >
> > > > 在 2021年9月28日，21:15，Piotr Nowojski <pn...@apache.org> 写道：
> > > >
> > > > Hi Leonard,
> > > >
> > > > Sorry for this causing you troubles, however that change in the
> return
> > > type
> > > > was done while this class still has been marked as
> > `@PublicEvolving`[1].
> > > As
> > > > of 1.13.x `SourceReaderContext` was `@PublicEvolving` and it was
> marked
> > > as
> > > > `@Public` only starting from Flink 1.14.0 [2]. Probably what confused
> > you
> > > > was that both of those changes (changing the return type and making
> it
> > > > `@Public`) happened in the same release.
> > > >
> > > > However, those changes (`SourceReaderContext` and
> > `ResolvedCatalogTable`)
> > > > should have been clearly mentioned in the release notes with an
> upgrade
> > > > guide.
> > > >
> > > > Best, Piotrek
> > > >
> > > > [1]
> > > >
> > >
> >
> https://github.com/apache/flink/commit/7f3636f6b4f8bac415a7db85917ad849636bd730#diff-a247a24ccd1afc07c5d690a8a58b1f6584329925fdf0d7dc89361b90d621b7f2R31
> > > > [2] https://issues.apache.org/jira/browse/FLINK-22357
> > > >
> > > > wt., 28 wrz 2021 o 14:49 Leonard Xu <xb...@gmail.com> napisał(a):
> > > >
> > > >>>>
> > > >>>> Not sure if this will happen in 1.15 already. We will needed
> > automated
> > > >>>> compatibility tests and a well-defined list of stable API.
> > > >>
> > > >>> We are
> > > >>> trying to provide forward compatibility: applications using
> `@Public`
> > > >> APIs
> > > >>> compiled against Flink 1.12.x, should work fine in Flink 1.13.x
> > > >>
> > > >> Unfortunately, I also meet forward compatibility issue, when I do
> the
> > > >> release 1.14 check, I try to use mysql-cdc connector[1] which
> compiled
> > > >> against 1.13.1in SQL Client, but it can not work in flink 1.14.0
> > > cluster,
> > > >> it failed due to the metric API compatibility broken.
> > > >>
> > > >> @Public
> > > >> public interface SourceReaderContext {
> > > >>
> > > >> MetricGroup metricGroup();
> > > >>
> > > >>
> > > >> @Public
> > > >> public interface SourceReaderContext {
> > > >>
> > > >> SourceReaderMetricGroup metricGroup();
> > > >>
> > > >>
> > > >> Shouldn't we mark it as @Deprecated and then delete it util 2.0.0
> > > version
> > > >> for @Public API as the our community rule [2] described? At least we
> > > should
> > > >> keep them across server minor versions (<major>.<minor>.<patch>).
> > > >>
> > > >> Although these changes can be tracked to voted FLIPs and it’s not
> the
> > > >> fault of a few developers, it show us the fact that we didn’t pay
> > enough
> > > >> attention to back compatibility/forward compatibility.
> > > >>
> > > >> Best,
> > > >> Leonard
> > > >> [1]
> > > >>
> > >
> >
> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
> > > >> [2]
> > > >>
> > https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> > > >>
> > > >>
> > >
> > >
> >
>
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Piotr Nowojski <pn...@apache.org>.
> - We don't have any safeguards for stable API breaks. Big +1 for Ingo's
effort with architectural tests [3].
I think we have a compile time checks for breaking changes in `@Public`
marked classes/interfaces using japicmp [1].
Piotrek
[1] https://github.com/apache/flink/blob/master/pom.xml#L2014:L2084
wt., 28 wrz 2021 o 15:59 David Morávek <dm...@apache.org> napisał(a):
> This is a super interesting topic and there is already a great discussion.
> Here are few thoughts:
>
> - There is a delicate balance between fast delivery of the new features and
> API stability. Even though we should be careful with breaking evolving
> interfaces, it shouldn't stop us from making fast progress / iterate on
> features.
> - There are two camps of users. One camp prefers more frequent releases /
> new features (early adopters) and second that prefer less frequent stable
> releases. There was already a great discussion about this at Flink 1.14
> thread [1].
> - We're already trying to be explicit about which API's may break via
> annotations and the feature radar [2]. Stability annotations are a well
> known concept used by many projects. I think we still can go bit further
> here and aim for an IDE support (for example usages of guava @Experimental
> interfaces get highlighted, raising more awareness about potential issues).
> I'm not sure how this IDE integration works though.
> - We don't have any safeguards for stable API breaks. Big +1 for Ingo's
> effort with architectural tests [3].
>
> [1]
>
> https://lists.apache.org/thread.html/r76e1cdba577c6f4d6c86b23fdaeb53c4e3744c20d0b3e850fc2e14a7%40%3Cdev.flink.apache.org%3E
> [2] https://flink.apache.org/roadmap.html
> [3] https://issues.apache.org/jira/browse/FLINK-24138
>
> Best,
> D.
>
> On Tue, Sep 28, 2021 at 3:49 PM Leonard Xu <xb...@gmail.com> wrote:
>
> > Thanks Piotr for the kindly reply, what confused me is that
> > `SourceReaderContext` was marked @Public when it was born in flink 1.11,
> > and then it was corrected to @PublicEvolving in 1.11 -_-, and finally it
> > was changed to @Public again...
> >
> > As Flink and Flink ecosystem(Flink CDC connectors) developer, I think
> what
> > we're discussing is meaningful and I’d like to help improve those Public
> > API check for those changes.
> > Chesnay’s tips is a good idea that maybe we can use the tool like japicmp
> > to do the check for every PR in CI phase.
> >
> > Best,
> > Leonard
> >
> >
> > > 在 2021年9月28日，21:15，Piotr Nowojski <pn...@apache.org> 写道：
> > >
> > > Hi Leonard,
> > >
> > > Sorry for this causing you troubles, however that change in the return
> > type
> > > was done while this class still has been marked as
> `@PublicEvolving`[1].
> > As
> > > of 1.13.x `SourceReaderContext` was `@PublicEvolving` and it was marked
> > as
> > > `@Public` only starting from Flink 1.14.0 [2]. Probably what confused
> you
> > > was that both of those changes (changing the return type and making it
> > > `@Public`) happened in the same release.
> > >
> > > However, those changes (`SourceReaderContext` and
> `ResolvedCatalogTable`)
> > > should have been clearly mentioned in the release notes with an upgrade
> > > guide.
> > >
> > > Best, Piotrek
> > >
> > > [1]
> > >
> >
> https://github.com/apache/flink/commit/7f3636f6b4f8bac415a7db85917ad849636bd730#diff-a247a24ccd1afc07c5d690a8a58b1f6584329925fdf0d7dc89361b90d621b7f2R31
> > > [2] https://issues.apache.org/jira/browse/FLINK-22357
> > >
> > > wt., 28 wrz 2021 o 14:49 Leonard Xu <xb...@gmail.com> napisał(a):
> > >
> > >>>>
> > >>>> Not sure if this will happen in 1.15 already. We will needed
> automated
> > >>>> compatibility tests and a well-defined list of stable API.
> > >>
> > >>> We are
> > >>> trying to provide forward compatibility: applications using `@Public`
> > >> APIs
> > >>> compiled against Flink 1.12.x, should work fine in Flink 1.13.x
> > >>
> > >> Unfortunately, I also meet forward compatibility issue, when I do the
> > >> release 1.14 check, I try to use mysql-cdc connector[1] which compiled
> > >> against 1.13.1in SQL Client, but it can not work in flink 1.14.0
> > cluster,
> > >> it failed due to the metric API compatibility broken.
> > >>
> > >> @Public
> > >> public interface SourceReaderContext {
> > >>
> > >> MetricGroup metricGroup();
> > >>
> > >>
> > >> @Public
> > >> public interface SourceReaderContext {
> > >>
> > >> SourceReaderMetricGroup metricGroup();
> > >>
> > >>
> > >> Shouldn't we mark it as @Deprecated and then delete it util 2.0.0
> > version
> > >> for @Public API as the our community rule [2] described? At least we
> > should
> > >> keep them across server minor versions (<major>.<minor>.<patch>).
> > >>
> > >> Although these changes can be tracked to voted FLIPs and it’s not the
> > >> fault of a few developers, it show us the fact that we didn’t pay
> enough
> > >> attention to back compatibility/forward compatibility.
> > >>
> > >> Best,
> > >> Leonard
> > >> [1]
> > >>
> >
> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
> > >> [2]
> > >>
> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> > >>
> > >>
> >
> >
>
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by David Morávek <dm...@apache.org>.
This is a super interesting topic and there is already a great discussion.
Here are few thoughts:
- There is a delicate balance between fast delivery of the new features and
API stability. Even though we should be careful with breaking evolving
interfaces, it shouldn't stop us from making fast progress / iterate on
features.
- There are two camps of users. One camp prefers more frequent releases /
new features (early adopters) and second that prefer less frequent stable
releases. There was already a great discussion about this at Flink 1.14
thread [1].
- We're already trying to be explicit about which API's may break via
annotations and the feature radar [2]. Stability annotations are a well
known concept used by many projects. I think we still can go bit further
here and aim for an IDE support (for example usages of guava @Experimental
interfaces get highlighted, raising more awareness about potential issues).
I'm not sure how this IDE integration works though.
- We don't have any safeguards for stable API breaks. Big +1 for Ingo's
effort with architectural tests [3].
[1]
https://lists.apache.org/thread.html/r76e1cdba577c6f4d6c86b23fdaeb53c4e3744c20d0b3e850fc2e14a7%40%3Cdev.flink.apache.org%3E
[2] https://flink.apache.org/roadmap.html
[3] https://issues.apache.org/jira/browse/FLINK-24138
Best,
D.
On Tue, Sep 28, 2021 at 3:49 PM Leonard Xu <xb...@gmail.com> wrote:
> Thanks Piotr for the kindly reply, what confused me is that
> `SourceReaderContext` was marked @Public when it was born in flink 1.11,
> and then it was corrected to @PublicEvolving in 1.11 -_-, and finally it
> was changed to @Public again...
>
> As Flink and Flink ecosystem(Flink CDC connectors) developer, I think what
> we're discussing is meaningful and I’d like to help improve those Public
> API check for those changes.
> Chesnay’s tips is a good idea that maybe we can use the tool like japicmp
> to do the check for every PR in CI phase.
>
> Best,
> Leonard
>
>
> > 在 2021年9月28日，21:15，Piotr Nowojski <pn...@apache.org> 写道：
> >
> > Hi Leonard,
> >
> > Sorry for this causing you troubles, however that change in the return
> type
> > was done while this class still has been marked as `@PublicEvolving`[1].
> As
> > of 1.13.x `SourceReaderContext` was `@PublicEvolving` and it was marked
> as
> > `@Public` only starting from Flink 1.14.0 [2]. Probably what confused you
> > was that both of those changes (changing the return type and making it
> > `@Public`) happened in the same release.
> >
> > However, those changes (`SourceReaderContext` and `ResolvedCatalogTable`)
> > should have been clearly mentioned in the release notes with an upgrade
> > guide.
> >
> > Best, Piotrek
> >
> > [1]
> >
> https://github.com/apache/flink/commit/7f3636f6b4f8bac415a7db85917ad849636bd730#diff-a247a24ccd1afc07c5d690a8a58b1f6584329925fdf0d7dc89361b90d621b7f2R31
> > [2] https://issues.apache.org/jira/browse/FLINK-22357
> >
> > wt., 28 wrz 2021 o 14:49 Leonard Xu <xb...@gmail.com> napisał(a):
> >
> >>>>
> >>>> Not sure if this will happen in 1.15 already. We will needed automated
> >>>> compatibility tests and a well-defined list of stable API.
> >>
> >>> We are
> >>> trying to provide forward compatibility: applications using `@Public`
> >> APIs
> >>> compiled against Flink 1.12.x, should work fine in Flink 1.13.x
> >>
> >> Unfortunately, I also meet forward compatibility issue, when I do the
> >> release 1.14 check, I try to use mysql-cdc connector[1] which compiled
> >> against 1.13.1in SQL Client, but it can not work in flink 1.14.0
> cluster,
> >> it failed due to the metric API compatibility broken.
> >>
> >> @Public
> >> public interface SourceReaderContext {
> >>
> >> MetricGroup metricGroup();
> >>
> >>
> >> @Public
> >> public interface SourceReaderContext {
> >>
> >> SourceReaderMetricGroup metricGroup();
> >>
> >>
> >> Shouldn't we mark it as @Deprecated and then delete it util 2.0.0
> version
> >> for @Public API as the our community rule [2] described? At least we
> should
> >> keep them across server minor versions (<major>.<minor>.<patch>).
> >>
> >> Although these changes can be tracked to voted FLIPs and it’s not the
> >> fault of a few developers, it show us the fact that we didn’t pay enough
> >> attention to back compatibility/forward compatibility.
> >>
> >> Best,
> >> Leonard
> >> [1]
> >>
> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
> >> [2]
> >> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
> >>
> >>
>
>
Re: The Apache Flink should pay more attention to ensuring API
compatibility.
Posted by Leonard Xu <xb...@gmail.com>.
Thanks Piotr for the kindly reply, what confused me is that `SourceReaderContext` was marked @Public when it was born in flink 1.11,
and then it was corrected to @PublicEvolving in 1.11 -_-, and finally it was changed to @Public again...
As Flink and Flink ecosystem(Flink CDC connectors) developer, I think what we're discussing is meaningful and I’d like to help improve those Public API check for those changes.
Chesnay’s tips is a good idea that maybe we can use the tool like japicmp to do the check for every PR in CI phase.
Best,
Leonard
> 在 2021年9月28日，21:15，Piotr Nowojski <pn...@apache.org> 写道：
>
> Hi Leonard,
>
> Sorry for this causing you troubles, however that change in the return type
> was done while this class still has been marked as `@PublicEvolving`[1]. As
> of 1.13.x `SourceReaderContext` was `@PublicEvolving` and it was marked as
> `@Public` only starting from Flink 1.14.0 [2]. Probably what confused you
> was that both of those changes (changing the return type and making it
> `@Public`) happened in the same release.
>
> However, those changes (`SourceReaderContext` and `ResolvedCatalogTable`)
> should have been clearly mentioned in the release notes with an upgrade
> guide.
>
> Best, Piotrek
>
> [1]
> https://github.com/apache/flink/commit/7f3636f6b4f8bac415a7db85917ad849636bd730#diff-a247a24ccd1afc07c5d690a8a58b1f6584329925fdf0d7dc89361b90d621b7f2R31
> [2] https://issues.apache.org/jira/browse/FLINK-22357
>
> wt., 28 wrz 2021 o 14:49 Leonard Xu <xb...@gmail.com> napisał(a):
>
>>>>
>>>> Not sure if this will happen in 1.15 already. We will needed automated
>>>> compatibility tests and a well-defined list of stable API.
>>
>>> We are
>>> trying to provide forward compatibility: applications using `@Public`
>> APIs
>>> compiled against Flink 1.12.x, should work fine in Flink 1.13.x
>>
>> Unfortunately, I also meet forward compatibility issue, when I do the
>> release 1.14 check, I try to use mysql-cdc connector[1] which compiled
>> against 1.13.1in SQL Client, but it can not work in flink 1.14.0 cluster,
>> it failed due to the metric API compatibility broken.
>>
>> @Public
>> public interface SourceReaderContext {
>>
>> MetricGroup metricGroup();
>>
>>
>> @Public
>> public interface SourceReaderContext {
>>
>> SourceReaderMetricGroup metricGroup();
>>
>>
>> Shouldn't we mark it as @Deprecated and then delete it util 2.0.0 version
>> for @Public API as the our community rule [2] described? At least we should
>> keep them across server minor versions (<major>.<minor>.<patch>).
>>
>> Although these changes can be tracked to voted FLIPs and it’s not the
>> fault of a few developers, it show us the fact that we didn’t pay enough
>> attention to back compatibility/forward compatibility.
>>
>> Best,
>> Leonard
>> [1]
>> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
>> [2]
>> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
>>
>>
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Piotr Nowojski <pn...@apache.org>.
Hi Leonard,
Sorry for this causing you troubles, however that change in the return type
was done while this class still has been marked as `@PublicEvolving`[1]. As
of 1.13.x `SourceReaderContext` was `@PublicEvolving` and it was marked as
`@Public` only starting from Flink 1.14.0 [2]. Probably what confused you
was that both of those changes (changing the return type and making it
`@Public`) happened in the same release.
However, those changes (`SourceReaderContext` and `ResolvedCatalogTable`)
should have been clearly mentioned in the release notes with an upgrade
guide.
Best, Piotrek
[1]
https://github.com/apache/flink/commit/7f3636f6b4f8bac415a7db85917ad849636bd730#diff-a247a24ccd1afc07c5d690a8a58b1f6584329925fdf0d7dc89361b90d621b7f2R31
[2] https://issues.apache.org/jira/browse/FLINK-22357
wt., 28 wrz 2021 o 14:49 Leonard Xu <xb...@gmail.com> napisał(a):
> >>
> >> Not sure if this will happen in 1.15 already. We will needed automated
> >> compatibility tests and a well-defined list of stable API.
>
> > We are
> > trying to provide forward compatibility: applications using `@Public`
> APIs
> > compiled against Flink 1.12.x, should work fine in Flink 1.13.x
>
> Unfortunately, I also meet forward compatibility issue, when I do the
> release 1.14 check, I try to use mysql-cdc connector[1] which compiled
> against 1.13.1in SQL Client, but it can not work in flink 1.14.0 cluster,
> it failed due to the metric API compatibility broken.
>
> @Public
> public interface SourceReaderContext {
>
> MetricGroup metricGroup();
>
>
> @Public
> public interface SourceReaderContext {
>
> SourceReaderMetricGroup metricGroup();
>
>
> Shouldn't we mark it as @Deprecated and then delete it util 2.0.0 version
> for @Public API as the our community rule [2] described? At least we should
> keep them across server minor versions (<major>.<minor>.<patch>).
>
> Although these changes can be tracked to voted FLIPs and it’s not the
> fault of a few developers, it show us the fact that we didn’t pay enough
> attention to back compatibility/forward compatibility.
>
> Best,
> Leonard
> [1]
> https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
> [2]
> https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
>
>
Re: The Apache Flink should pay more attention to ensuring API
compatibility.
Posted by Leonard Xu <xb...@gmail.com>.
>>
>> Not sure if this will happen in 1.15 already. We will needed automated
>> compatibility tests and a well-defined list of stable API.
> We are
> trying to provide forward compatibility: applications using `@Public` APIs
> compiled against Flink 1.12.x, should work fine in Flink 1.13.x
Unfortunately, I also meet forward compatibility issue, when I do the release 1.14 check, I try to use mysql-cdc connector[1] which compiled against 1.13.1in SQL Client, but it can not work in flink 1.14.0 cluster, it failed due to the metric API compatibility broken.
@Public
public interface SourceReaderContext {
MetricGroup metricGroup();
@Public
public interface SourceReaderContext {
SourceReaderMetricGroup metricGroup();
Shouldn't we mark it as @Deprecated and then delete it util 2.0.0 version for @Public API as the our community rule [2] described? At least we should keep them across server minor versions (<major>.<minor>.<patch>).
Although these changes can be tracked to voted FLIPs and it’s not the fault of a few developers, it show us the fact that we didn’t pay enough attention to back compatibility/forward compatibility.
Best,
Leonard
[1] https://github.com/ververica/flink-cdc-connectors/tree/master/flink-connector-mysql-cdc
[2] https://cwiki.apache.org/confluence/display/FLINK/Stability+Annotations
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Piotr Nowojski <pn...@apache.org>.
Hi,
> we find the iceberg-flink-runtime.jar built
> by flink 1.13 cannot works fine in flink 1.12 clusters because of the
basic
> API compatibility was break when iterating flink 1.12 to flink1.13.2:
Apart from this being `@PublicEvolving` one thing to note here is that we
do not guarantee such compatibility even for `@Public` interfaces. We are
trying to provide forward compatibility: applications using `@Public` APIs
compiled against Flink 1.12.x, should work fine in Flink 1.13.x. We do not
guarantee it the other way: applications compiled against Flink 1.13.x are
not intended to work with Flink 1.12.x. For example quite often we are
extending `@Public` API, by for example adding an overloaded method and
obviously such change won't work in an older Flink cluster, while it will
work with an older application.
Best,
Piotrek
wt., 28 wrz 2021 o 12:55 Timo Walther <tw...@apache.org> napisał(a):
> I opened https://issues.apache.org/jira/browse/FLINK-24396 to track this
> effort.
>
> Not sure if this will happen in 1.15 already. We will needed automated
> compatibility tests and a well-defined list of stable API.
>
> We can also do this incrementally and start with the interfaces for
> connectors.
>
> Regards,
> Timo
>
>
> On 28.09.21 11:47, Leonard Xu wrote:
> > Thanks @peninx for the feedback, this will definitely help the flink
> community.
> >
> > Recently, we also developed a series of connectors in Flink CDC
> project[1]. They are based on flink version 1.13.1, but many users still
> use flink version 1.12.* in production. They have encountered similar
> problems, and it is difficult to upgrade the flink cluster version within
> their company.
> >
> > Therefore, things like directly changing CatalogTable to
> ResolvedCatalogTable should not happen, we should have marked it as
> @Deprecated and keep at least one version for compatibility.
> >
> > In one word, it's valuable feedback that we will pay more attention to
> API compatibility.
> >
> > Best,
> > Leonard
> >
> > [1] https://github.com/ververica/flink-cdc-connectors
> >
> >> 在 2021年9月28日，17:16，Jeff Zhang <zj...@gmail.com> 写道：
> >>
> >> I believe I mentioned this before in the community, we (Zeppelin) use
> flink
> >> api as well and would like to support multiple versions of flink in one
> >> zeppelin version. For now we have to use reflection to achieve that.
> >>
> >> https://github.com/apache/zeppelin/tree/master/flink
> >>
> >>
> >> OpenInx <op...@gmail.com> 于2021年9月28日周二 下午5:10写道：
> >>
> >>> Thanks for the information, Martijin & Timo !
> >>>
> >>>> Since implementing a connector is not straightforward, we were
> expecting
> >>> that not many users implement custom connectors.
> >>>
> >>> Currently, the apache iceberg & hudi are heavily depending on the
> >>> PublicEvolving API for their flink connectors. I think apache hudi
> even
> >>> uses more public API than iceberg to implement their relatively
> complicated
> >>> flink sink DAG, I think Danny Chen [1] may want to provide more
> input. API
> >>> compatibility has become one of the core reasons that downstream
> projects
> >>> maintainers vote to support a release or not because bandwidth from the
> >>> downstream projects are limited and we maintainers need to balance
> between
> >>> the community requirements and cost. A great compatible flink release
> will
> >>> greatly save the maintenance cost (especially we flink release often )
> and
> >>> we are also glad to make it a longer life cycle.
> >>>
> >>>> We therefore consider this part as a kind of "second level API" for
> which
> >>> we can evolve quicker.
> >>>
> >>> That sounds great ! I'm glad to see that we are making the API more
> >>> friendly !
> >>>
> >>> [1]. https://github.com/danny0405
> >>>
> >>>
> >>>
> >>> On Tue, Sep 28, 2021 at 3:52 PM Timo Walther <tw...@apache.org>
> wrote:
> >>>
> >>>> Hi Zheng,
> >>>>
> >>>> I'm very sorry for the inconvenience that we have caused with our API
> >>>> changes. We are trying our best to avoid API breaking changes. Thanks
> >>>> for giving us feedback.
> >>>>
> >>>> There has been a reason why Table API was marked as @PublicEvolving
> >>>> instead of @Public. Over the last two years, we have basically
> rewritten
> >>>> the entire API [1] to digest the Blink merge and making the Table API
> >>>> stable and ready for the future. We tried our best to give users 1-2
> >>>> releases time to upgrade their implementations whenever we deprecated
> >>>> API but we were aware that this might cause frustration, but hopefully
> >>>> for the greater good. We have reworked type system, Catalog API,
> schema,
> >>>> source/sinks, functions and much more. Flink 1.14 will hopefully be
> the
> >>>> last release with major API changes. We could also mark most Table API
> >>>> interfaces as `@Public` in 1.15.
> >>>>
> >>>> For your mentioned incompatibility, I agree that the change from
> >>>> CatalogTable to ResolvedCatalogTable was not very nice. Since
> >>>> implementing a connector is not straight forward, we were expecting
> that
> >>>> not many users implement custom connectors. We therefore consider this
> >>>> part as kind of "second level API" for which we can evolve quicker. A
> >>>> `context.getCatalogTable().getSchema()` should still work for 1.12 and
> >>>> 1.13, at least that was the goal.
> >>>>
> >>>> Thanks again for the feedback. It was a good reminder and we will pay
> >>>> more attention to this.
> >>>>
> >>>> Regards,
> >>>> Timo
> >>>>
> >>>> [1]
> >>>>
> >>>>
> >>>
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-32%3A+Restructure+flink-table+for+future+contributions
> >>>>
> >>>>
> >>>> On 28.09.21 08:40, Martijn Visser wrote:
> >>>>> Hi Zheng,
> >>>>>
> >>>>> Thanks for reaching out and sharing your frustration. No feelings are
> >>>> hurt
> >>>>> and feedback is always welcome, because that's the only way we can
> >>>> improve
> >>>>> for the future. API compatibility is a really important thing for us
> >>>> while
> >>>>> also improving and building new capabilities. Let me investigate a
> bit
> >>>> what
> >>>>> happened on our end, share that and then try to get some learnings
> out
> >>> of
> >>>>> it for the future. I'll get back to you in a couple of days.
> >>>>>
> >>>>> Best regards,
> >>>>>
> >>>>> Martijn Visser | Product Manager
> >>>>>
> >>>>> martijn@ververica.com
> >>>>>
> >>>>> <https://www.ververica.com/>
> >>>>>
> >>>>>
> >>>>> Follow us @VervericaData
> >>>>>
> >>>>> --
> >>>>>
> >>>>> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
> >>>>> Conference
> >>>>>
> >>>>> Stream Processing | Event Driven | Real Time
> >>>>>
> >>>>>
> >>>>> On Tue, 28 Sept 2021 at 07:39, OpenInx <op...@gmail.com> wrote:
> >>>>>
> >>>>>> Sorry about my unfriendly tone of the last e-mail, I got frustrated
> >>>> about
> >>>>>> the experience of maintaining the project which is closely with
> Flink.
> >>>> My
> >>>>>> intention was trying to remind everyone to be careful about API
> >>>>>> compatibility and didn't really watch out for the tone I used.
> >>>>>>
> >>>>>> Hope that doesn't hurt anyone's feelings.
> >>>>>>
> >>>>>> On Tue, Sep 28, 2021 at 12:33 PM OpenInx <op...@gmail.com> wrote:
> >>>>>>
> >>>>>>> Hi Dev
> >>>>>>>
> >>>>>>> We are trying to upgrade the flink version from 1.12.0 to 1.13.2 in
> >>>>>> apache
> >>>>>>> iceberg project ( https://github.com/apache/iceberg/pull/3116),
> but
> >>>>>> it's
> >>>>>>> not a great experience. We expect to support both flink1.12 and
> >>>>>> flink1.13
> >>>>>>> in an iceberg-flink module without using the new API of flink1.13
> for
> >>>>>>> saving maintenance cost, but we find the iceberg-flink-runtime.jar
> >>>> built
> >>>>>>> by flink 1.13 cannot works fine in flink 1.12 clusters because of
> the
> >>>>>> basic
> >>>>>>> API compatibility was break when iterating flink 1.12 to
> flink1.13.2:
> >>>>>>>
> >>>>>>> (The following are copied from the iceberg issue:
> >>>>>>>
> https://github.com/apache/iceberg/issues/3187#issuecomment-928755046
> >>> )
> >>>>>>>
> >>>>>>> Thanks for the report, @Reo-LEI ! I think this issue was introduced
> >>>> from
> >>>>>>> this apache flink PR (
> >>>>>>>
> >>>>>>
> >>>>
> >>>
> https://github.com/apache/flink/pull/15316/files#diff-bd276ed951054125b39428ee61de103d9c7832246398f01514a574bb8e51757cR74
> >>>>>> )
> >>>>>>> and FLINK-21913 (https://issues.apache.org/jira/browse/FLINK-21913
> ),
> >>>> it
> >>>>>>> just changed the returned data type from CatalogTable to
> >>>>>>> ResolvedCatalogTable without any compatibility guarantee. In this
> >>> case,
> >>>>>> the
> >>>>>>> iceberg-flink-runtime jar which is compiled from apache flink 1.13
> >>> will
> >>>>>>> include the ResovledCatalogTable class inside it. Finally when we
> >>>> package
> >>>>>>> this jar and submit the flink job to flink 1.12, the above
> >>>> compatibility
> >>>>>>> issue happen.
> >>>>>>>
> >>>>>>> As we all know, the DynamicTableFactory (
> >>>>>>>
> >>>>>>
> >>>>
> >>>
> https://github.com/apache/flink/blob/99c2a415e9eeefafacf70762b6f54070f7911ceb/flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/DynamicTableFactory.java
> >>>>>> )
> >>>>>>> is a basic API which almost all flink connectors are built on top
> of
> >>>> it.
> >>>>>>> The breaking compatibility makes the downstream projects really
> hard
> >>> to
> >>>>>>> deliver better compatibility to users, unless we iceberg maintain
> >>>>>> different
> >>>>>>> modules for each maintained flink version (That's not the thing
> that
> >>> we
> >>>>>>> want to do).
> >>>>>>>
> >>>>>>> The last flink upgrading work is also not a good experience (See
> the
> >>>>>>> discussion (https://github.com/apache/iceberg/pull/1956) and
> >>> comment (
> >>>>>>> https://github.com/apache/iceberg/pull/1956#discussion_r546534299)
> >>> ),
> >>>>>>> because the flink 1.12 also breaks several API that was annotated
> >>>>>>> PublicEvolving in flink 1.11.0, that becomes one of the most
> >>> important
> >>>>>>> reasons leading to the conclusion that stops support flink 1.11.0
> in
> >>>> our
> >>>>>>> apache iceberg branch ( Supporting new features [such as flip-27
> >>>> unified
> >>>>>>> iceberg source/sink] that depends the API introduced in flink 1.12
> is
> >>>>>>> another reason). To better support the compatibility of downstream
> >>>>>> systems
> >>>>>>> and delivering better experience to flink users, I will strongly
> >>>> suggest
> >>>>>>> the Apache Flink community to pay more attention to ensuring API
> >>>>>>> compatibility.
> >>>>>>>
> >>>>>>>
> >>>>>>> Zheng Hu (openinx)
> >>>>>>>
> >>>>>>> Thanks.
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>>
> >>>
> >>
> >>
> >> --
> >> Best Regards
> >>
> >> Jeff Zhang
> >
>
>
Re: The Apache Flink should pay more attention to ensuring API
compatibility.
Posted by Timo Walther <tw...@apache.org>.
I opened https://issues.apache.org/jira/browse/FLINK-24396 to track this
effort.
Not sure if this will happen in 1.15 already. We will needed automated
compatibility tests and a well-defined list of stable API.
We can also do this incrementally and start with the interfaces for
connectors.
Regards,
Timo
On 28.09.21 11:47, Leonard Xu wrote:
> Thanks @peninx for the feedback, this will definitely help the flink community.
>
> Recently, we also developed a series of connectors in Flink CDC project[1]. They are based on flink version 1.13.1, but many users still use flink version 1.12.* in production. They have encountered similar problems, and it is difficult to upgrade the flink cluster version within their company.
>
> Therefore, things like directly changing CatalogTable to ResolvedCatalogTable should not happen, we should have marked it as @Deprecated and keep at least one version for compatibility.
>
> In one word, it's valuable feedback that we will pay more attention to API compatibility.
>
> Best,
> Leonard
>
> [1] https://github.com/ververica/flink-cdc-connectors
>
>> 在 2021年9月28日，17:16，Jeff Zhang <zj...@gmail.com> 写道：
>>
>> I believe I mentioned this before in the community, we (Zeppelin) use flink
>> api as well and would like to support multiple versions of flink in one
>> zeppelin version. For now we have to use reflection to achieve that.
>>
>> https://github.com/apache/zeppelin/tree/master/flink
>>
>>
>> OpenInx <op...@gmail.com> 于2021年9月28日周二 下午5:10写道：
>>
>>> Thanks for the information, Martijin & Timo !
>>>
>>>> Since implementing a connector is not straightforward, we were expecting
>>> that not many users implement custom connectors.
>>>
>>> Currently, the apache iceberg & hudi are heavily depending on the
>>> PublicEvolving API for their flink connectors. I think apache hudi even
>>> uses more public API than iceberg to implement their relatively complicated
>>> flink sink DAG, I think Danny Chen [1] may want to provide more input. API
>>> compatibility has become one of the core reasons that downstream projects
>>> maintainers vote to support a release or not because bandwidth from the
>>> downstream projects are limited and we maintainers need to balance between
>>> the community requirements and cost. A great compatible flink release will
>>> greatly save the maintenance cost (especially we flink release often ) and
>>> we are also glad to make it a longer life cycle.
>>>
>>>> We therefore consider this part as a kind of "second level API" for which
>>> we can evolve quicker.
>>>
>>> That sounds great ! I'm glad to see that we are making the API more
>>> friendly !
>>>
>>> [1]. https://github.com/danny0405
>>>
>>>
>>>
>>> On Tue, Sep 28, 2021 at 3:52 PM Timo Walther <tw...@apache.org> wrote:
>>>
>>>> Hi Zheng,
>>>>
>>>> I'm very sorry for the inconvenience that we have caused with our API
>>>> changes. We are trying our best to avoid API breaking changes. Thanks
>>>> for giving us feedback.
>>>>
>>>> There has been a reason why Table API was marked as @PublicEvolving
>>>> instead of @Public. Over the last two years, we have basically rewritten
>>>> the entire API [1] to digest the Blink merge and making the Table API
>>>> stable and ready for the future. We tried our best to give users 1-2
>>>> releases time to upgrade their implementations whenever we deprecated
>>>> API but we were aware that this might cause frustration, but hopefully
>>>> for the greater good. We have reworked type system, Catalog API, schema,
>>>> source/sinks, functions and much more. Flink 1.14 will hopefully be the
>>>> last release with major API changes. We could also mark most Table API
>>>> interfaces as `@Public` in 1.15.
>>>>
>>>> For your mentioned incompatibility, I agree that the change from
>>>> CatalogTable to ResolvedCatalogTable was not very nice. Since
>>>> implementing a connector is not straight forward, we were expecting that
>>>> not many users implement custom connectors. We therefore consider this
>>>> part as kind of "second level API" for which we can evolve quicker. A
>>>> `context.getCatalogTable().getSchema()` should still work for 1.12 and
>>>> 1.13, at least that was the goal.
>>>>
>>>> Thanks again for the feedback. It was a good reminder and we will pay
>>>> more attention to this.
>>>>
>>>> Regards,
>>>> Timo
>>>>
>>>> [1]
>>>>
>>>>
>>> https://cwiki.apache.org/confluence/display/FLINK/FLIP-32%3A+Restructure+flink-table+for+future+contributions
>>>>
>>>>
>>>> On 28.09.21 08:40, Martijn Visser wrote:
>>>>> Hi Zheng,
>>>>>
>>>>> Thanks for reaching out and sharing your frustration. No feelings are
>>>> hurt
>>>>> and feedback is always welcome, because that's the only way we can
>>>> improve
>>>>> for the future. API compatibility is a really important thing for us
>>>> while
>>>>> also improving and building new capabilities. Let me investigate a bit
>>>> what
>>>>> happened on our end, share that and then try to get some learnings out
>>> of
>>>>> it for the future. I'll get back to you in a couple of days.
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Martijn Visser | Product Manager
>>>>>
>>>>> martijn@ververica.com
>>>>>
>>>>> <https://www.ververica.com/>
>>>>>
>>>>>
>>>>> Follow us @VervericaData
>>>>>
>>>>> --
>>>>>
>>>>> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
>>>>> Conference
>>>>>
>>>>> Stream Processing | Event Driven | Real Time
>>>>>
>>>>>
>>>>> On Tue, 28 Sept 2021 at 07:39, OpenInx <op...@gmail.com> wrote:
>>>>>
>>>>>> Sorry about my unfriendly tone of the last e-mail, I got frustrated
>>>> about
>>>>>> the experience of maintaining the project which is closely with Flink.
>>>> My
>>>>>> intention was trying to remind everyone to be careful about API
>>>>>> compatibility and didn't really watch out for the tone I used.
>>>>>>
>>>>>> Hope that doesn't hurt anyone's feelings.
>>>>>>
>>>>>> On Tue, Sep 28, 2021 at 12:33 PM OpenInx <op...@gmail.com> wrote:
>>>>>>
>>>>>>> Hi Dev
>>>>>>>
>>>>>>> We are trying to upgrade the flink version from 1.12.0 to 1.13.2 in
>>>>>> apache
>>>>>>> iceberg project ( https://github.com/apache/iceberg/pull/3116), but
>>>>>> it's
>>>>>>> not a great experience. We expect to support both flink1.12 and
>>>>>> flink1.13
>>>>>>> in an iceberg-flink module without using the new API of flink1.13 for
>>>>>>> saving maintenance cost, but we find the iceberg-flink-runtime.jar
>>>> built
>>>>>>> by flink 1.13 cannot works fine in flink 1.12 clusters because of the
>>>>>> basic
>>>>>>> API compatibility was break when iterating flink 1.12 to flink1.13.2:
>>>>>>>
>>>>>>> (The following are copied from the iceberg issue:
>>>>>>> https://github.com/apache/iceberg/issues/3187#issuecomment-928755046
>>> )
>>>>>>>
>>>>>>> Thanks for the report, @Reo-LEI ! I think this issue was introduced
>>>> from
>>>>>>> this apache flink PR (
>>>>>>>
>>>>>>
>>>>
>>> https://github.com/apache/flink/pull/15316/files#diff-bd276ed951054125b39428ee61de103d9c7832246398f01514a574bb8e51757cR74
>>>>>> )
>>>>>>> and FLINK-21913 (https://issues.apache.org/jira/browse/FLINK-21913),
>>>> it
>>>>>>> just changed the returned data type from CatalogTable to
>>>>>>> ResolvedCatalogTable without any compatibility guarantee. In this
>>> case,
>>>>>> the
>>>>>>> iceberg-flink-runtime jar which is compiled from apache flink 1.13
>>> will
>>>>>>> include the ResovledCatalogTable class inside it. Finally when we
>>>> package
>>>>>>> this jar and submit the flink job to flink 1.12, the above
>>>> compatibility
>>>>>>> issue happen.
>>>>>>>
>>>>>>> As we all know, the DynamicTableFactory (
>>>>>>>
>>>>>>
>>>>
>>> https://github.com/apache/flink/blob/99c2a415e9eeefafacf70762b6f54070f7911ceb/flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/DynamicTableFactory.java
>>>>>> )
>>>>>>> is a basic API which almost all flink connectors are built on top of
>>>> it.
>>>>>>> The breaking compatibility makes the downstream projects really hard
>>> to
>>>>>>> deliver better compatibility to users, unless we iceberg maintain
>>>>>> different
>>>>>>> modules for each maintained flink version (That's not the thing that
>>> we
>>>>>>> want to do).
>>>>>>>
>>>>>>> The last flink upgrading work is also not a good experience (See the
>>>>>>> discussion (https://github.com/apache/iceberg/pull/1956) and
>>> comment (
>>>>>>> https://github.com/apache/iceberg/pull/1956#discussion_r546534299)
>>> ),
>>>>>>> because the flink 1.12 also breaks several API that was annotated
>>>>>>> PublicEvolving in flink 1.11.0, that becomes one of the most
>>> important
>>>>>>> reasons leading to the conclusion that stops support flink 1.11.0 in
>>>> our
>>>>>>> apache iceberg branch ( Supporting new features [such as flip-27
>>>> unified
>>>>>>> iceberg source/sink] that depends the API introduced in flink 1.12 is
>>>>>>> another reason). To better support the compatibility of downstream
>>>>>> systems
>>>>>>> and delivering better experience to flink users, I will strongly
>>>> suggest
>>>>>>> the Apache Flink community to pay more attention to ensuring API
>>>>>>> compatibility.
>>>>>>>
>>>>>>>
>>>>>>> Zheng Hu (openinx)
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>> --
>> Best Regards
>>
>> Jeff Zhang
>
Re: The Apache Flink should pay more attention to ensuring API
compatibility.
Posted by Leonard Xu <xb...@gmail.com>.
Thanks @peninx for the feedback, this will definitely help the flink community.
Recently, we also developed a series of connectors in Flink CDC project[1]. They are based on flink version 1.13.1, but many users still use flink version 1.12.* in production. They have encountered similar problems, and it is difficult to upgrade the flink cluster version within their company.
Therefore, things like directly changing CatalogTable to ResolvedCatalogTable should not happen, we should have marked it as @Deprecated and keep at least one version for compatibility.
In one word, it's valuable feedback that we will pay more attention to API compatibility.
Best,
Leonard
[1] https://github.com/ververica/flink-cdc-connectors
> 在 2021年9月28日，17:16，Jeff Zhang <zj...@gmail.com> 写道：
>
> I believe I mentioned this before in the community, we (Zeppelin) use flink
> api as well and would like to support multiple versions of flink in one
> zeppelin version. For now we have to use reflection to achieve that.
>
> https://github.com/apache/zeppelin/tree/master/flink
>
>
> OpenInx <op...@gmail.com> 于2021年9月28日周二 下午5:10写道：
>
>> Thanks for the information, Martijin & Timo !
>>
>>> Since implementing a connector is not straightforward, we were expecting
>> that not many users implement custom connectors.
>>
>> Currently, the apache iceberg & hudi are heavily depending on the
>> PublicEvolving API for their flink connectors. I think apache hudi even
>> uses more public API than iceberg to implement their relatively complicated
>> flink sink DAG, I think Danny Chen [1] may want to provide more input. API
>> compatibility has become one of the core reasons that downstream projects
>> maintainers vote to support a release or not because bandwidth from the
>> downstream projects are limited and we maintainers need to balance between
>> the community requirements and cost. A great compatible flink release will
>> greatly save the maintenance cost (especially we flink release often ) and
>> we are also glad to make it a longer life cycle.
>>
>>> We therefore consider this part as a kind of "second level API" for which
>> we can evolve quicker.
>>
>> That sounds great ! I'm glad to see that we are making the API more
>> friendly !
>>
>> [1]. https://github.com/danny0405
>>
>>
>>
>> On Tue, Sep 28, 2021 at 3:52 PM Timo Walther <tw...@apache.org> wrote:
>>
>>> Hi Zheng,
>>>
>>> I'm very sorry for the inconvenience that we have caused with our API
>>> changes. We are trying our best to avoid API breaking changes. Thanks
>>> for giving us feedback.
>>>
>>> There has been a reason why Table API was marked as @PublicEvolving
>>> instead of @Public. Over the last two years, we have basically rewritten
>>> the entire API [1] to digest the Blink merge and making the Table API
>>> stable and ready for the future. We tried our best to give users 1-2
>>> releases time to upgrade their implementations whenever we deprecated
>>> API but we were aware that this might cause frustration, but hopefully
>>> for the greater good. We have reworked type system, Catalog API, schema,
>>> source/sinks, functions and much more. Flink 1.14 will hopefully be the
>>> last release with major API changes. We could also mark most Table API
>>> interfaces as `@Public` in 1.15.
>>>
>>> For your mentioned incompatibility, I agree that the change from
>>> CatalogTable to ResolvedCatalogTable was not very nice. Since
>>> implementing a connector is not straight forward, we were expecting that
>>> not many users implement custom connectors. We therefore consider this
>>> part as kind of "second level API" for which we can evolve quicker. A
>>> `context.getCatalogTable().getSchema()` should still work for 1.12 and
>>> 1.13, at least that was the goal.
>>>
>>> Thanks again for the feedback. It was a good reminder and we will pay
>>> more attention to this.
>>>
>>> Regards,
>>> Timo
>>>
>>> [1]
>>>
>>>
>> https://cwiki.apache.org/confluence/display/FLINK/FLIP-32%3A+Restructure+flink-table+for+future+contributions
>>>
>>>
>>> On 28.09.21 08:40, Martijn Visser wrote:
>>>> Hi Zheng,
>>>>
>>>> Thanks for reaching out and sharing your frustration. No feelings are
>>> hurt
>>>> and feedback is always welcome, because that's the only way we can
>>> improve
>>>> for the future. API compatibility is a really important thing for us
>>> while
>>>> also improving and building new capabilities. Let me investigate a bit
>>> what
>>>> happened on our end, share that and then try to get some learnings out
>> of
>>>> it for the future. I'll get back to you in a couple of days.
>>>>
>>>> Best regards,
>>>>
>>>> Martijn Visser | Product Manager
>>>>
>>>> martijn@ververica.com
>>>>
>>>> <https://www.ververica.com/>
>>>>
>>>>
>>>> Follow us @VervericaData
>>>>
>>>> --
>>>>
>>>> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
>>>> Conference
>>>>
>>>> Stream Processing | Event Driven | Real Time
>>>>
>>>>
>>>> On Tue, 28 Sept 2021 at 07:39, OpenInx <op...@gmail.com> wrote:
>>>>
>>>>> Sorry about my unfriendly tone of the last e-mail, I got frustrated
>>> about
>>>>> the experience of maintaining the project which is closely with Flink.
>>> My
>>>>> intention was trying to remind everyone to be careful about API
>>>>> compatibility and didn't really watch out for the tone I used.
>>>>>
>>>>> Hope that doesn't hurt anyone's feelings.
>>>>>
>>>>> On Tue, Sep 28, 2021 at 12:33 PM OpenInx <op...@gmail.com> wrote:
>>>>>
>>>>>> Hi Dev
>>>>>>
>>>>>> We are trying to upgrade the flink version from 1.12.0 to 1.13.2 in
>>>>> apache
>>>>>> iceberg project ( https://github.com/apache/iceberg/pull/3116), but
>>>>> it's
>>>>>> not a great experience. We expect to support both flink1.12 and
>>>>> flink1.13
>>>>>> in an iceberg-flink module without using the new API of flink1.13 for
>>>>>> saving maintenance cost, but we find the iceberg-flink-runtime.jar
>>> built
>>>>>> by flink 1.13 cannot works fine in flink 1.12 clusters because of the
>>>>> basic
>>>>>> API compatibility was break when iterating flink 1.12 to flink1.13.2:
>>>>>>
>>>>>> (The following are copied from the iceberg issue:
>>>>>> https://github.com/apache/iceberg/issues/3187#issuecomment-928755046
>> )
>>>>>>
>>>>>> Thanks for the report, @Reo-LEI ! I think this issue was introduced
>>> from
>>>>>> this apache flink PR (
>>>>>>
>>>>>
>>>
>> https://github.com/apache/flink/pull/15316/files#diff-bd276ed951054125b39428ee61de103d9c7832246398f01514a574bb8e51757cR74
>>>>> )
>>>>>> and FLINK-21913 (https://issues.apache.org/jira/browse/FLINK-21913),
>>> it
>>>>>> just changed the returned data type from CatalogTable to
>>>>>> ResolvedCatalogTable without any compatibility guarantee. In this
>> case,
>>>>> the
>>>>>> iceberg-flink-runtime jar which is compiled from apache flink 1.13
>> will
>>>>>> include the ResovledCatalogTable class inside it. Finally when we
>>> package
>>>>>> this jar and submit the flink job to flink 1.12, the above
>>> compatibility
>>>>>> issue happen.
>>>>>>
>>>>>> As we all know, the DynamicTableFactory (
>>>>>>
>>>>>
>>>
>> https://github.com/apache/flink/blob/99c2a415e9eeefafacf70762b6f54070f7911ceb/flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/DynamicTableFactory.java
>>>>> )
>>>>>> is a basic API which almost all flink connectors are built on top of
>>> it.
>>>>>> The breaking compatibility makes the downstream projects really hard
>> to
>>>>>> deliver better compatibility to users, unless we iceberg maintain
>>>>> different
>>>>>> modules for each maintained flink version (That's not the thing that
>> we
>>>>>> want to do).
>>>>>>
>>>>>> The last flink upgrading work is also not a good experience (See the
>>>>>> discussion (https://github.com/apache/iceberg/pull/1956) and
>> comment (
>>>>>> https://github.com/apache/iceberg/pull/1956#discussion_r546534299)
>> ),
>>>>>> because the flink 1.12 also breaks several API that was annotated
>>>>>> PublicEvolving in flink 1.11.0, that becomes one of the most
>> important
>>>>>> reasons leading to the conclusion that stops support flink 1.11.0 in
>>> our
>>>>>> apache iceberg branch ( Supporting new features [such as flip-27
>>> unified
>>>>>> iceberg source/sink] that depends the API introduced in flink 1.12 is
>>>>>> another reason). To better support the compatibility of downstream
>>>>> systems
>>>>>> and delivering better experience to flink users, I will strongly
>>> suggest
>>>>>> the Apache Flink community to pay more attention to ensuring API
>>>>>> compatibility.
>>>>>>
>>>>>>
>>>>>> Zheng Hu (openinx)
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>
> --
> Best Regards
>
> Jeff Zhang
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Jeff Zhang <zj...@gmail.com>.
I believe I mentioned this before in the community, we (Zeppelin) use flink
api as well and would like to support multiple versions of flink in one
zeppelin version. For now we have to use reflection to achieve that.
https://github.com/apache/zeppelin/tree/master/flink
OpenInx <op...@gmail.com> 于2021年9月28日周二 下午5:10写道：
> Thanks for the information, Martijin & Timo !
>
> > Since implementing a connector is not straightforward, we were expecting
> that not many users implement custom connectors.
>
> Currently, the apache iceberg & hudi are heavily depending on the
> PublicEvolving API for their flink connectors. I think apache hudi even
> uses more public API than iceberg to implement their relatively complicated
> flink sink DAG, I think Danny Chen [1] may want to provide more input. API
> compatibility has become one of the core reasons that downstream projects
> maintainers vote to support a release or not because bandwidth from the
> downstream projects are limited and we maintainers need to balance between
> the community requirements and cost. A great compatible flink release will
> greatly save the maintenance cost (especially we flink release often ) and
> we are also glad to make it a longer life cycle.
>
> > We therefore consider this part as a kind of "second level API" for which
> we can evolve quicker.
>
> That sounds great ! I'm glad to see that we are making the API more
> friendly !
>
> [1]. https://github.com/danny0405
>
>
>
> On Tue, Sep 28, 2021 at 3:52 PM Timo Walther <tw...@apache.org> wrote:
>
> > Hi Zheng,
> >
> > I'm very sorry for the inconvenience that we have caused with our API
> > changes. We are trying our best to avoid API breaking changes. Thanks
> > for giving us feedback.
> >
> > There has been a reason why Table API was marked as @PublicEvolving
> > instead of @Public. Over the last two years, we have basically rewritten
> > the entire API [1] to digest the Blink merge and making the Table API
> > stable and ready for the future. We tried our best to give users 1-2
> > releases time to upgrade their implementations whenever we deprecated
> > API but we were aware that this might cause frustration, but hopefully
> > for the greater good. We have reworked type system, Catalog API, schema,
> > source/sinks, functions and much more. Flink 1.14 will hopefully be the
> > last release with major API changes. We could also mark most Table API
> > interfaces as `@Public` in 1.15.
> >
> > For your mentioned incompatibility, I agree that the change from
> > CatalogTable to ResolvedCatalogTable was not very nice. Since
> > implementing a connector is not straight forward, we were expecting that
> > not many users implement custom connectors. We therefore consider this
> > part as kind of "second level API" for which we can evolve quicker. A
> > `context.getCatalogTable().getSchema()` should still work for 1.12 and
> > 1.13, at least that was the goal.
> >
> > Thanks again for the feedback. It was a good reminder and we will pay
> > more attention to this.
> >
> > Regards,
> > Timo
> >
> > [1]
> >
> >
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-32%3A+Restructure+flink-table+for+future+contributions
> >
> >
> > On 28.09.21 08:40, Martijn Visser wrote:
> > > Hi Zheng,
> > >
> > > Thanks for reaching out and sharing your frustration. No feelings are
> > hurt
> > > and feedback is always welcome, because that's the only way we can
> > improve
> > > for the future. API compatibility is a really important thing for us
> > while
> > > also improving and building new capabilities. Let me investigate a bit
> > what
> > > happened on our end, share that and then try to get some learnings out
> of
> > > it for the future. I'll get back to you in a couple of days.
> > >
> > > Best regards,
> > >
> > > Martijn Visser | Product Manager
> > >
> > > martijn@ververica.com
> > >
> > > <https://www.ververica.com/>
> > >
> > >
> > > Follow us @VervericaData
> > >
> > > --
> > >
> > > Join Flink Forward <https://flink-forward.org/> - The Apache Flink
> > > Conference
> > >
> > > Stream Processing | Event Driven | Real Time
> > >
> > >
> > > On Tue, 28 Sept 2021 at 07:39, OpenInx <op...@gmail.com> wrote:
> > >
> > >> Sorry about my unfriendly tone of the last e-mail, I got frustrated
> > about
> > >> the experience of maintaining the project which is closely with Flink.
> > My
> > >> intention was trying to remind everyone to be careful about API
> > >> compatibility and didn't really watch out for the tone I used.
> > >>
> > >> Hope that doesn't hurt anyone's feelings.
> > >>
> > >> On Tue, Sep 28, 2021 at 12:33 PM OpenInx <op...@gmail.com> wrote:
> > >>
> > >>> Hi Dev
> > >>>
> > >>> We are trying to upgrade the flink version from 1.12.0 to 1.13.2 in
> > >> apache
> > >>> iceberg project ( https://github.com/apache/iceberg/pull/3116), but
> > >> it's
> > >>> not a great experience. We expect to support both flink1.12 and
> > >> flink1.13
> > >>> in an iceberg-flink module without using the new API of flink1.13 for
> > >>> saving maintenance cost, but we find the iceberg-flink-runtime.jar
> > built
> > >>> by flink 1.13 cannot works fine in flink 1.12 clusters because of the
> > >> basic
> > >>> API compatibility was break when iterating flink 1.12 to flink1.13.2:
> > >>>
> > >>> (The following are copied from the iceberg issue:
> > >>> https://github.com/apache/iceberg/issues/3187#issuecomment-928755046
> )
> > >>>
> > >>> Thanks for the report, @Reo-LEI ! I think this issue was introduced
> > from
> > >>> this apache flink PR (
> > >>>
> > >>
> >
> https://github.com/apache/flink/pull/15316/files#diff-bd276ed951054125b39428ee61de103d9c7832246398f01514a574bb8e51757cR74
> > >> )
> > >>> and FLINK-21913 (https://issues.apache.org/jira/browse/FLINK-21913),
> > it
> > >>> just changed the returned data type from CatalogTable to
> > >>> ResolvedCatalogTable without any compatibility guarantee. In this
> case,
> > >> the
> > >>> iceberg-flink-runtime jar which is compiled from apache flink 1.13
> will
> > >>> include the ResovledCatalogTable class inside it. Finally when we
> > package
> > >>> this jar and submit the flink job to flink 1.12, the above
> > compatibility
> > >>> issue happen.
> > >>>
> > >>> As we all know, the DynamicTableFactory (
> > >>>
> > >>
> >
> https://github.com/apache/flink/blob/99c2a415e9eeefafacf70762b6f54070f7911ceb/flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/DynamicTableFactory.java
> > >> )
> > >>> is a basic API which almost all flink connectors are built on top of
> > it.
> > >>> The breaking compatibility makes the downstream projects really hard
> to
> > >>> deliver better compatibility to users, unless we iceberg maintain
> > >> different
> > >>> modules for each maintained flink version (That's not the thing that
> we
> > >>> want to do).
> > >>>
> > >>> The last flink upgrading work is also not a good experience (See the
> > >>> discussion (https://github.com/apache/iceberg/pull/1956) and
> comment (
> > >>> https://github.com/apache/iceberg/pull/1956#discussion_r546534299)
> ),
> > >>> because the flink 1.12 also breaks several API that was annotated
> > >>> PublicEvolving in flink 1.11.0, that becomes one of the most
> important
> > >>> reasons leading to the conclusion that stops support flink 1.11.0 in
> > our
> > >>> apache iceberg branch ( Supporting new features [such as flip-27
> > unified
> > >>> iceberg source/sink] that depends the API introduced in flink 1.12 is
> > >>> another reason). To better support the compatibility of downstream
> > >> systems
> > >>> and delivering better experience to flink users, I will strongly
> > suggest
> > >>> the Apache Flink community to pay more attention to ensuring API
> > >>> compatibility.
> > >>>
> > >>>
> > >>> Zheng Hu (openinx)
> > >>>
> > >>> Thanks.
> > >>>
> > >>
> > >
> >
> >
>
--
Best Regards
Jeff Zhang
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by OpenInx <op...@gmail.com>.
Thanks for the information, Martijin & Timo !
> Since implementing a connector is not straightforward, we were expecting
that not many users implement custom connectors.
Currently, the apache iceberg & hudi are heavily depending on the
PublicEvolving API for their flink connectors. I think apache hudi even
uses more public API than iceberg to implement their relatively complicated
flink sink DAG, I think Danny Chen [1] may want to provide more input. API
compatibility has become one of the core reasons that downstream projects
maintainers vote to support a release or not because bandwidth from the
downstream projects are limited and we maintainers need to balance between
the community requirements and cost. A great compatible flink release will
greatly save the maintenance cost (especially we flink release often ) and
we are also glad to make it a longer life cycle.
> We therefore consider this part as a kind of "second level API" for which
we can evolve quicker.
That sounds great ! I'm glad to see that we are making the API more
friendly !
[1]. https://github.com/danny0405
On Tue, Sep 28, 2021 at 3:52 PM Timo Walther <tw...@apache.org> wrote:
> Hi Zheng,
>
> I'm very sorry for the inconvenience that we have caused with our API
> changes. We are trying our best to avoid API breaking changes. Thanks
> for giving us feedback.
>
> There has been a reason why Table API was marked as @PublicEvolving
> instead of @Public. Over the last two years, we have basically rewritten
> the entire API [1] to digest the Blink merge and making the Table API
> stable and ready for the future. We tried our best to give users 1-2
> releases time to upgrade their implementations whenever we deprecated
> API but we were aware that this might cause frustration, but hopefully
> for the greater good. We have reworked type system, Catalog API, schema,
> source/sinks, functions and much more. Flink 1.14 will hopefully be the
> last release with major API changes. We could also mark most Table API
> interfaces as `@Public` in 1.15.
>
> For your mentioned incompatibility, I agree that the change from
> CatalogTable to ResolvedCatalogTable was not very nice. Since
> implementing a connector is not straight forward, we were expecting that
> not many users implement custom connectors. We therefore consider this
> part as kind of "second level API" for which we can evolve quicker. A
> `context.getCatalogTable().getSchema()` should still work for 1.12 and
> 1.13, at least that was the goal.
>
> Thanks again for the feedback. It was a good reminder and we will pay
> more attention to this.
>
> Regards,
> Timo
>
> [1]
>
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-32%3A+Restructure+flink-table+for+future+contributions
>
>
> On 28.09.21 08:40, Martijn Visser wrote:
> > Hi Zheng,
> >
> > Thanks for reaching out and sharing your frustration. No feelings are
> hurt
> > and feedback is always welcome, because that's the only way we can
> improve
> > for the future. API compatibility is a really important thing for us
> while
> > also improving and building new capabilities. Let me investigate a bit
> what
> > happened on our end, share that and then try to get some learnings out of
> > it for the future. I'll get back to you in a couple of days.
> >
> > Best regards,
> >
> > Martijn Visser | Product Manager
> >
> > martijn@ververica.com
> >
> > <https://www.ververica.com/>
> >
> >
> > Follow us @VervericaData
> >
> > --
> >
> > Join Flink Forward <https://flink-forward.org/> - The Apache Flink
> > Conference
> >
> > Stream Processing | Event Driven | Real Time
> >
> >
> > On Tue, 28 Sept 2021 at 07:39, OpenInx <op...@gmail.com> wrote:
> >
> >> Sorry about my unfriendly tone of the last e-mail, I got frustrated
> about
> >> the experience of maintaining the project which is closely with Flink.
> My
> >> intention was trying to remind everyone to be careful about API
> >> compatibility and didn't really watch out for the tone I used.
> >>
> >> Hope that doesn't hurt anyone's feelings.
> >>
> >> On Tue, Sep 28, 2021 at 12:33 PM OpenInx <op...@gmail.com> wrote:
> >>
> >>> Hi Dev
> >>>
> >>> We are trying to upgrade the flink version from 1.12.0 to 1.13.2 in
> >> apache
> >>> iceberg project ( https://github.com/apache/iceberg/pull/3116), but
> >> it's
> >>> not a great experience. We expect to support both flink1.12 and
> >> flink1.13
> >>> in an iceberg-flink module without using the new API of flink1.13 for
> >>> saving maintenance cost, but we find the iceberg-flink-runtime.jar
> built
> >>> by flink 1.13 cannot works fine in flink 1.12 clusters because of the
> >> basic
> >>> API compatibility was break when iterating flink 1.12 to flink1.13.2:
> >>>
> >>> (The following are copied from the iceberg issue:
> >>> https://github.com/apache/iceberg/issues/3187#issuecomment-928755046)
> >>>
> >>> Thanks for the report, @Reo-LEI ! I think this issue was introduced
> from
> >>> this apache flink PR (
> >>>
> >>
> https://github.com/apache/flink/pull/15316/files#diff-bd276ed951054125b39428ee61de103d9c7832246398f01514a574bb8e51757cR74
> >> )
> >>> and FLINK-21913 (https://issues.apache.org/jira/browse/FLINK-21913),
> it
> >>> just changed the returned data type from CatalogTable to
> >>> ResolvedCatalogTable without any compatibility guarantee. In this case,
> >> the
> >>> iceberg-flink-runtime jar which is compiled from apache flink 1.13 will
> >>> include the ResovledCatalogTable class inside it. Finally when we
> package
> >>> this jar and submit the flink job to flink 1.12, the above
> compatibility
> >>> issue happen.
> >>>
> >>> As we all know, the DynamicTableFactory (
> >>>
> >>
> https://github.com/apache/flink/blob/99c2a415e9eeefafacf70762b6f54070f7911ceb/flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/DynamicTableFactory.java
> >> )
> >>> is a basic API which almost all flink connectors are built on top of
> it.
> >>> The breaking compatibility makes the downstream projects really hard to
> >>> deliver better compatibility to users, unless we iceberg maintain
> >> different
> >>> modules for each maintained flink version (That's not the thing that we
> >>> want to do).
> >>>
> >>> The last flink upgrading work is also not a good experience (See the
> >>> discussion (https://github.com/apache/iceberg/pull/1956) and comment (
> >>> https://github.com/apache/iceberg/pull/1956#discussion_r546534299) ),
> >>> because the flink 1.12 also breaks several API that was annotated
> >>> PublicEvolving in flink 1.11.0, that becomes one of the most important
> >>> reasons leading to the conclusion that stops support flink 1.11.0 in
> our
> >>> apache iceberg branch ( Supporting new features [such as flip-27
> unified
> >>> iceberg source/sink] that depends the API introduced in flink 1.12 is
> >>> another reason). To better support the compatibility of downstream
> >> systems
> >>> and delivering better experience to flink users, I will strongly
> suggest
> >>> the Apache Flink community to pay more attention to ensuring API
> >>> compatibility.
> >>>
> >>>
> >>> Zheng Hu (openinx)
> >>>
> >>> Thanks.
> >>>
> >>
> >
>
>
Re: The Apache Flink should pay more attention to ensuring API
compatibility.
Posted by Timo Walther <tw...@apache.org>.
Hi Zheng,
I'm very sorry for the inconvenience that we have caused with our API
changes. We are trying our best to avoid API breaking changes. Thanks
for giving us feedback.
There has been a reason why Table API was marked as @PublicEvolving
instead of @Public. Over the last two years, we have basically rewritten
the entire API [1] to digest the Blink merge and making the Table API
stable and ready for the future. We tried our best to give users 1-2
releases time to upgrade their implementations whenever we deprecated
API but we were aware that this might cause frustration, but hopefully
for the greater good. We have reworked type system, Catalog API, schema,
source/sinks, functions and much more. Flink 1.14 will hopefully be the
last release with major API changes. We could also mark most Table API
interfaces as `@Public` in 1.15.
For your mentioned incompatibility, I agree that the change from
CatalogTable to ResolvedCatalogTable was not very nice. Since
implementing a connector is not straight forward, we were expecting that
not many users implement custom connectors. We therefore consider this
part as kind of "second level API" for which we can evolve quicker. A
`context.getCatalogTable().getSchema()` should still work for 1.12 and
1.13, at least that was the goal.
Thanks again for the feedback. It was a good reminder and we will pay
more attention to this.
Regards,
Timo
[1]
https://cwiki.apache.org/confluence/display/FLINK/FLIP-32%3A+Restructure+flink-table+for+future+contributions
On 28.09.21 08:40, Martijn Visser wrote:
> Hi Zheng,
>
> Thanks for reaching out and sharing your frustration. No feelings are hurt
> and feedback is always welcome, because that's the only way we can improve
> for the future. API compatibility is a really important thing for us while
> also improving and building new capabilities. Let me investigate a bit what
> happened on our end, share that and then try to get some learnings out of
> it for the future. I'll get back to you in a couple of days.
>
> Best regards,
>
> Martijn Visser | Product Manager
>
> martijn@ververica.com
>
> <https://www.ververica.com/>
>
>
> Follow us @VervericaData
>
> --
>
> Join Flink Forward <https://flink-forward.org/> - The Apache Flink
> Conference
>
> Stream Processing | Event Driven | Real Time
>
>
> On Tue, 28 Sept 2021 at 07:39, OpenInx <op...@gmail.com> wrote:
>
>> Sorry about my unfriendly tone of the last e-mail, I got frustrated about
>> the experience of maintaining the project which is closely with Flink. My
>> intention was trying to remind everyone to be careful about API
>> compatibility and didn't really watch out for the tone I used.
>>
>> Hope that doesn't hurt anyone's feelings.
>>
>> On Tue, Sep 28, 2021 at 12:33 PM OpenInx <op...@gmail.com> wrote:
>>
>>> Hi Dev
>>>
>>> We are trying to upgrade the flink version from 1.12.0 to 1.13.2 in
>> apache
>>> iceberg project ( https://github.com/apache/iceberg/pull/3116), but
>> it's
>>> not a great experience. We expect to support both flink1.12 and
>> flink1.13
>>> in an iceberg-flink module without using the new API of flink1.13 for
>>> saving maintenance cost, but we find the iceberg-flink-runtime.jar built
>>> by flink 1.13 cannot works fine in flink 1.12 clusters because of the
>> basic
>>> API compatibility was break when iterating flink 1.12 to flink1.13.2:
>>>
>>> (The following are copied from the iceberg issue:
>>> https://github.com/apache/iceberg/issues/3187#issuecomment-928755046)
>>>
>>> Thanks for the report, @Reo-LEI ! I think this issue was introduced from
>>> this apache flink PR (
>>>
>> https://github.com/apache/flink/pull/15316/files#diff-bd276ed951054125b39428ee61de103d9c7832246398f01514a574bb8e51757cR74
>> )
>>> and FLINK-21913 (https://issues.apache.org/jira/browse/FLINK-21913), it
>>> just changed the returned data type from CatalogTable to
>>> ResolvedCatalogTable without any compatibility guarantee. In this case,
>> the
>>> iceberg-flink-runtime jar which is compiled from apache flink 1.13 will
>>> include the ResovledCatalogTable class inside it. Finally when we package
>>> this jar and submit the flink job to flink 1.12, the above compatibility
>>> issue happen.
>>>
>>> As we all know, the DynamicTableFactory (
>>>
>> https://github.com/apache/flink/blob/99c2a415e9eeefafacf70762b6f54070f7911ceb/flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/DynamicTableFactory.java
>> )
>>> is a basic API which almost all flink connectors are built on top of it.
>>> The breaking compatibility makes the downstream projects really hard to
>>> deliver better compatibility to users, unless we iceberg maintain
>> different
>>> modules for each maintained flink version (That's not the thing that we
>>> want to do).
>>>
>>> The last flink upgrading work is also not a good experience (See the
>>> discussion (https://github.com/apache/iceberg/pull/1956) and comment (
>>> https://github.com/apache/iceberg/pull/1956#discussion_r546534299) ),
>>> because the flink 1.12 also breaks several API that was annotated
>>> PublicEvolving in flink 1.11.0, that becomes one of the most important
>>> reasons leading to the conclusion that stops support flink 1.11.0 in our
>>> apache iceberg branch ( Supporting new features [such as flip-27 unified
>>> iceberg source/sink] that depends the API introduced in flink 1.12 is
>>> another reason). To better support the compatibility of downstream
>> systems
>>> and delivering better experience to flink users, I will strongly suggest
>>> the Apache Flink community to pay more attention to ensuring API
>>> compatibility.
>>>
>>>
>>> Zheng Hu (openinx)
>>>
>>> Thanks.
>>>
>>
>
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by Martijn Visser <ma...@ververica.com>.
Hi Zheng,
Thanks for reaching out and sharing your frustration. No feelings are hurt
and feedback is always welcome, because that's the only way we can improve
for the future. API compatibility is a really important thing for us while
also improving and building new capabilities. Let me investigate a bit what
happened on our end, share that and then try to get some learnings out of
it for the future. I'll get back to you in a couple of days.
Best regards,
Martijn Visser | Product Manager
martijn@ververica.com
<https://www.ververica.com/>
Follow us @VervericaData
--
Join Flink Forward <https://flink-forward.org/> - The Apache Flink
Conference
Stream Processing | Event Driven | Real Time
On Tue, 28 Sept 2021 at 07:39, OpenInx <op...@gmail.com> wrote:
> Sorry about my unfriendly tone of the last e-mail, I got frustrated about
> the experience of maintaining the project which is closely with Flink. My
> intention was trying to remind everyone to be careful about API
> compatibility and didn't really watch out for the tone I used.
>
> Hope that doesn't hurt anyone's feelings.
>
> On Tue, Sep 28, 2021 at 12:33 PM OpenInx <op...@gmail.com> wrote:
>
> > Hi Dev
> >
> > We are trying to upgrade the flink version from 1.12.0 to 1.13.2 in
> apache
> > iceberg project ( https://github.com/apache/iceberg/pull/3116), but
> it's
> > not a great experience. We expect to support both flink1.12 and
> flink1.13
> > in an iceberg-flink module without using the new API of flink1.13 for
> > saving maintenance cost, but we find the iceberg-flink-runtime.jar built
> > by flink 1.13 cannot works fine in flink 1.12 clusters because of the
> basic
> > API compatibility was break when iterating flink 1.12 to flink1.13.2:
> >
> > (The following are copied from the iceberg issue:
> > https://github.com/apache/iceberg/issues/3187#issuecomment-928755046)
> >
> > Thanks for the report, @Reo-LEI ! I think this issue was introduced from
> > this apache flink PR (
> >
> https://github.com/apache/flink/pull/15316/files#diff-bd276ed951054125b39428ee61de103d9c7832246398f01514a574bb8e51757cR74
> )
> > and FLINK-21913 (https://issues.apache.org/jira/browse/FLINK-21913), it
> > just changed the returned data type from CatalogTable to
> > ResolvedCatalogTable without any compatibility guarantee. In this case,
> the
> > iceberg-flink-runtime jar which is compiled from apache flink 1.13 will
> > include the ResovledCatalogTable class inside it. Finally when we package
> > this jar and submit the flink job to flink 1.12, the above compatibility
> > issue happen.
> >
> > As we all know, the DynamicTableFactory (
> >
> https://github.com/apache/flink/blob/99c2a415e9eeefafacf70762b6f54070f7911ceb/flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/DynamicTableFactory.java
> )
> > is a basic API which almost all flink connectors are built on top of it.
> > The breaking compatibility makes the downstream projects really hard to
> > deliver better compatibility to users, unless we iceberg maintain
> different
> > modules for each maintained flink version (That's not the thing that we
> > want to do).
> >
> > The last flink upgrading work is also not a good experience (See the
> > discussion (https://github.com/apache/iceberg/pull/1956) and comment (
> > https://github.com/apache/iceberg/pull/1956#discussion_r546534299) ),
> > because the flink 1.12 also breaks several API that was annotated
> > PublicEvolving in flink 1.11.0, that becomes one of the most important
> > reasons leading to the conclusion that stops support flink 1.11.0 in our
> > apache iceberg branch ( Supporting new features [such as flip-27 unified
> > iceberg source/sink] that depends the API introduced in flink 1.12 is
> > another reason). To better support the compatibility of downstream
> systems
> > and delivering better experience to flink users, I will strongly suggest
> > the Apache Flink community to pay more attention to ensuring API
> > compatibility.
> >
> >
> > Zheng Hu (openinx)
> >
> > Thanks.
> >
>
Re: The Apache Flink should pay more attention to ensuring API compatibility.
Posted by OpenInx <op...@gmail.com>.
Sorry about my unfriendly tone of the last e-mail, I got frustrated about
the experience of maintaining the project which is closely with Flink. My
intention was trying to remind everyone to be careful about API
compatibility and didn't really watch out for the tone I used.
Hope that doesn't hurt anyone's feelings.
On Tue, Sep 28, 2021 at 12:33 PM OpenInx <op...@gmail.com> wrote:
> Hi Dev
>
> We are trying to upgrade the flink version from 1.12.0 to 1.13.2 in apache
> iceberg project ( https://github.com/apache/iceberg/pull/3116), but it's
> not a great experience. We expect to support both flink1.12 and flink1.13
> in an iceberg-flink module without using the new API of flink1.13 for
> saving maintenance cost, but we find the iceberg-flink-runtime.jar built
> by flink 1.13 cannot works fine in flink 1.12 clusters because of the basic
> API compatibility was break when iterating flink 1.12 to flink1.13.2:
>
> (The following are copied from the iceberg issue:
> https://github.com/apache/iceberg/issues/3187#issuecomment-928755046)
>
> Thanks for the report, @Reo-LEI ! I think this issue was introduced from
> this apache flink PR (
> https://github.com/apache/flink/pull/15316/files#diff-bd276ed951054125b39428ee61de103d9c7832246398f01514a574bb8e51757cR74)
> and FLINK-21913 (https://issues.apache.org/jira/browse/FLINK-21913), it
> just changed the returned data type from CatalogTable to
> ResolvedCatalogTable without any compatibility guarantee. In this case, the
> iceberg-flink-runtime jar which is compiled from apache flink 1.13 will
> include the ResovledCatalogTable class inside it. Finally when we package
> this jar and submit the flink job to flink 1.12, the above compatibility
> issue happen.
>
> As we all know, the DynamicTableFactory (
> https://github.com/apache/flink/blob/99c2a415e9eeefafacf70762b6f54070f7911ceb/flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/DynamicTableFactory.java)
> is a basic API which almost all flink connectors are built on top of it.
> The breaking compatibility makes the downstream projects really hard to
> deliver better compatibility to users, unless we iceberg maintain different
> modules for each maintained flink version (That's not the thing that we
> want to do).
>
> The last flink upgrading work is also not a good experience (See the
> discussion (https://github.com/apache/iceberg/pull/1956) and comment (
> https://github.com/apache/iceberg/pull/1956#discussion_r546534299) ),
> because the flink 1.12 also breaks several API that was annotated
> PublicEvolving in flink 1.11.0, that becomes one of the most important
> reasons leading to the conclusion that stops support flink 1.11.0 in our
> apache iceberg branch ( Supporting new features [such as flip-27 unified
> iceberg source/sink] that depends the API introduced in flink 1.12 is
> another reason). To better support the compatibility of downstream systems
> and delivering better experience to flink users, I will strongly suggest
> the Apache Flink community to pay more attention to ensuring API
> compatibility.
>
>
> Zheng Hu (openinx)
>
> Thanks.
>