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 <imj...@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 <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 > >>> > > > > > >> > >>> > > > > > >> > >>> > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >> >
