On Wed, May 13, 2020 at 1:02 PM Piotr Nowojski <pi...@ververica.com> wrote:
> Hi, > > > For the future, it would be great to eliminate the possibility of > > incompatible changes to user-facing classes for patch releases though. > > Running japicmp as part of CI might be a good option. > > I guess the whole point of PublicEvolving classes is to allow us for the > breaking compatibility changes. > > The question here is whether such changes should occur in *patch* releases. I suspect most users would prefer stronger guarantees. Another example of "breaking" change that affects Beam (though class OptimizerPlanEnvironment isn't covered by annotation and it only affects test code): https://github.com/apache/beam/pull/11683 Piotrek > > > On 13 May 2020, at 18:29, Thomas Weise <t...@apache.org> wrote: > > > > For release branches, it would be good to not exclude PublicEvolving from > > checking, by default [1]. > > > > There may be instances where it is necessary to make exceptions to fix a > > critical bug, but those could be whitelisted. > > > > Similarly, it might be good to blacklist dependency changes for patch > > releases, by default. > > > > [1] > > > https://github.com/apache/flink/blob/e1e7d7f7ecc080c850a264021bf1b20e3d27d373/pom.xml#L1959 > > > > On Wed, May 13, 2020 at 9:16 AM Chesnay Schepler <ches...@apache.org> > wrote: > > > >> We are running japicmp in CI; we just don't check anything that is > >> PublicEvolving. > >> > >> On 13/05/2020 18:04, Thomas Weise wrote: > >>> +1 for the release note addition > >>> > >>> Historically Flink X.Y.0 releases have been prone to issues like this. > >> And > >>> I suspect that many users would prefer to upgrade to X.Y.1 due to other > >>> bugs that need to be shaken out. > >>> > >>> For the future, it would be great to eliminate the possibility of > >>> incompatible changes to user-facing classes for patch releases though. > >>> Running japicmp as part of CI might be a good option. > >>> > >>> Thanks, > >>> Thomas > >>> > >>> On Wed, May 13, 2020 at 7:54 AM Seth Wiesman <sjwies...@gmail.com> > >> wrote: > >>> > >>>> +1 Ufuk's pragmatic solution to update the release notes with a public > >>>> notice, I have commented on the release notes PR. > >>>> > >>>> https://github.com/apache/flink-web/pull/330 > >>>> > >>>> Seth > >>>> > >>>> On Wed, May 13, 2020 at 8:30 AM Chesnay Schepler <ches...@apache.org> > >>>> wrote: > >>>> > >>>>> IIRC @PublicEvolving had no /real/ guarantees, from the 1.0.0 > >>>> announcement: > >>>>> /"Flink 1.0.0 introduces interface stability annotations for API > >> classes > >>>>> and methods. Interfaces defined as //|@Public|//are guaranteed to > >> remain > >>>>> stable across all releases of the 1.x series. The > >>>>> //|@PublicEvolving|//annotation marks API features that may be > subject > >>>>> to change in future versions."/ > >>>>> > >>>>> 1) We don't have a process for promoting things to @Public because we > >>>>> have actually never done that. Based on our current rules, it is an > >>>>> addition to the public API (technically even more so than most > FLIPs), > >>>>> and hence should go through a FLIP and vote. > >>>>> Ideally we evaluate existing @PublicEvolving API's for promotion on > >> each > >>>>> release. > >>>>> > >>>>> 2) Yes, I think this is an excellent idea; as Ufuk said users should > >> not > >>>>> be worried about upgrading to the next minor version. Not sure how > easy > >>>>> this is to implement; but it basically means either modifying or > adding > >>>>> new new japicmp execution when forking the release-X.Y branches. > >>>>> > >>>>> On 13/05/2020 15:19, Till Rohrmann wrote: > >>>>>> A small addendum: The project currently enforces only binary > >>>>> compatibility > >>>>>> for classes which are annotated with @Public. The StreamingFileSink > is > >>>>>> annotated with @PublicEvolving. > >>>>>> > >>>>>> I am not sure whether the community properly defined what kind of > >>>>>> stability/compatibility guarantees we provide for @PublicEvolving > >>>>> classes. > >>>>>> We can derive two follow-up discussions from this: > >>>>>> > >>>>>> 1) Should the StreamingFileSink be made @Public? Should > >>>>>> other @PublicEvolving classes be promoted? What is the process here? > >>>>>> > >>>>>> 2) Should we enforce binary compatibility of @PublicEvolving classes > >>>>>> between bug fix releases and allow breaking changes between > >> minor/major > >>>>>> releases? > >>>>>> > >>>>>> Cheers, > >>>>>> Till > >>>>>> > >>>>>> On Wed, May 13, 2020 at 2:30 PM Ufuk Celebi <u...@apache.org> wrote: > >>>>>> > >>>>>>> Thanks for the analysis Till. > >>>>>>> > >>>>>>> 1/ I think breaking binary compatibility is acceptable between > minor > >>>>>>> releases (1.10 -> 1.11) as you suggested since the API is marked > >>>>>>> as @PublicEvolving. > >>>>>>> > >>>>>>> 2/ I'm quite torn about how to proceed with the 1.10.x patch > release, > >>>>> but > >>>>>>> slightly leaning towards the "pragmatic" solution of keeping the > >>>> change > >>>>> and > >>>>>>> adding a big warning to the 1.10.1 release announcement*. What do > >>>> others > >>>>>>> think? If we do want to revert the change and follow up with a > 1.10.2 > >>>>>>> release we should do it _before_ we announce 1.10.1 publicly. Doing > >> it > >>>>>>> after 1.10.1 has been announced would only cause more friction for > >>>>> users. > >>>>>>> – Ufuk > >>>>>>> > >>>>>>> *I hope we don't lose user trust with this. I really would like > users > >>>>> to be > >>>>>>> able to upgrade to the latest patch release without hesitation. > >>>>>>> > >>>>>>> On Wed, May 13, 2020 at 11:45 AM Till Rohrmann < > trohrm...@apache.org > >>> > >>>>>>> wrote: > >>>>>>> > >>>>>>>> Thanks for reporting this issue Seth. This is indeed a big > problem. > >>>>>>>> > >>>>>>>> I looked into the problem and it seems we have the following > >>>> situation: > >>>>>>>> # 1.9.x --> 1.10.0 > >>>>>>>> > >>>>>>>> There is an API breaking change between 1.9.x and 1.10 due > >>>> FLINK-13864 > >>>>>>>> because it introduced another generic parameter. I expected only > few > >>>>>>> people > >>>>>>>> will be affected by this because one would have to store the > builder > >>>>> in a > >>>>>>>> variable. > >>>>>>>> > >>>>>>>> 1.9.x is binary compatible with 1.10.0. > >>>>>>>> > >>>>>>>> # 1.10.0 -> 1.10.1 > >>>>>>>> > >>>>>>>> There is no API breaking change between 1.10.0 and 1.10.1. > >>>>>>>> > >>>>>>>> I changed the return type of the StreamingFileSink.forRowFormat > and > >>>>>>>> .forBulkFormat to a subtype of StreamingFileSink.BulkFormatBuilder > >> in > >>>>>>>> FLINK-16684. This causes the java.lang.NoSuchMethodError and the > >>>> binary > >>>>>>>> incompatibility between 1.10.0 and 1.10.1. This is should not > happen > >>>>> for > >>>>>>>> bug fix releases. > >>>>>>>> > >>>>>>>> W/o FLINK-16684, the StreamingFileSink builders cannot be used > with > >>>>>>> Flink's > >>>>>>>> Scala API. > >>>>>>>> > >>>>>>>> # Options > >>>>>>>> > >>>>>>>> I think we should keep the fix for 1.11.0 and add a release note > >> that > >>>>> we > >>>>>>>> are violating binary compatibility between 1.10 and 1.11. > >>>>>>>> > >>>>>>>> Now the question is what to do with the 1.10.x branch: > >>>>>>>> > >>>>>>>> a) We can revert the change to re-establish binary compatibility > >>>>> between > >>>>>>>> 1.9.x, 1.10.0 and 1.10.2 but not between 1.10.1 and 1.10.2. This > >>>> would > >>>>>>> also > >>>>>>>> imply that we cannot use the StreamingFileSink builders with the > >>>> Scala > >>>>>>> API. > >>>>>>>> b) Keep the change and add a release note that our users need to > >>>>>>> recompile > >>>>>>>> their jobs. This would allow Flink 1.10.x with x >= 1 users to use > >>>>>>>> StreamingFileSink builders with the Scala API. > >>>>>>>> > >>>>>>>> I am not entirely sure which need weighs more: binary > compatibility > >>>>>>> between > >>>>>>>> bug fix releases or a working API (small subset of it). Another > >>>> aspect > >>>>> to > >>>>>>>> consider is how many people will migrate from 1.10.0 to 1.10.1 > >>>> compared > >>>>>>> to > >>>>>>>> 1.y to 1.10.1 with y <= 9. If the former is very small then one > >> might > >>>>>>> make > >>>>>>>> a case for keeping the change. > >>>>>>>> > >>>>>>>> What do the others think? > >>>>>>>> > >>>>>>>> https://issues.apache.org/jira/browse/FLINK-13864 > >>>>>>>> https://issues.apache.org/jira/browse/FLINK-16684 > >>>>>>>> > >>>>>>>> Cheers, > >>>>>>>> Till > >>>>>>>> > >>>>>>>> On Tue, May 12, 2020 at 7:26 PM Thomas Weise <t...@apache.org> > >> wrote: > >>>>>>>> > >>>>>>>>> We also noticed that and had to make an adjustment downstream. > >>>>>>>>> > >>>>>>>>> It would be good to mention this in the release notes (if that's > >> not > >>>>>>>>> already the case). > >>>>>>>>> > >>>>>>>>> Thomas > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> On Tue, May 12, 2020 at 10:06 AM Seth Wiesman < > sjwies...@gmail.com > >>> > >>>>>>>> wrote: > >>>>>>>>>> Hi Everyone, > >>>>>>>>>> > >>>>>>>>>> I realize I'm about 7 hours late but I just realized there is a > >>>>>>>> breaking > >>>>>>>>>> API change in 1.10.1. FLINK-16684 changes the API of the > streaming > >>>>>>> file > >>>>>>>>>> sink in a binary incompatible way. Since the release has been > >>>>>>> approved > >>>>>>>>> I'm > >>>>>>>>>> not sure the correct course of action but I wanted to bring this > >> to > >>>>>>> the > >>>>>>>>>> communities attention. > >>>>>>>>>> > >>>>>>>>>> Seth > >>>>>>>>>> > >>>>>>>>>> https://issues.apache.org/jira/browse/FLINK-16684 > >>>>>>>>>> > >>>>> > >> > >> > >
