> 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 > > > > >> > > > > >> > > > > > > > > > > > > > >
