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 <imj...@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 <i...@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 <imj...@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 >> <xbjt...@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 <open...@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 <d...@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 <pnowoj...@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 <d...@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 <xbjt...@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 <pnowoj...@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 <xbjt...@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 >> > > > > > >> >> > > > > > >> >> > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> >
