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