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