I agree with the pragmatic solution. Concerning the stability guarantees I will start a separate discussion thread whether to provide stricter guarantees or not.
Cheers, Till On Wed, May 13, 2020 at 10:13 PM Thomas Weise <t...@apache.org> wrote: > 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 > > >>>>>>>>>> > > >>>>> > > >> > > >> > > > > >
