Thanks a lot for all your input. I hope that I could resolve most of your
concerns. I will now start a vote on this FLIP.
Cheers,
Till
On Tue, Dec 7, 2021 at 4:28 PM Till Rohrmann <trohrm...@apache.org> wrote:
> Hi Fabian,
>
> Thanks for your input. Adding the definition of the different labels to
> our documentation is definitely part of this FLIP.
>
> I agree that deprecation is related to API stability. For the sake of
> keeping this FLIP narrowly scoped I left it out, though. For the time being
> I suggest that we stick to our current deprecation process that says that a
> deprecated API can only be removed after it has been deprecated for at
> least one release. Of course, deprecating and removing a @PublicEvolving
> API can only happen in minor or major releases, for example. This process
> is a bit stricter than what the API stability guarantees require because
> with the API stability alone we could remove the APIs directly w/o the
> deprecation period. Other details I would leave for a follow up FLIP
> which formally defines the deprecation process.
>
> Cheers,
> Till
>
> On Tue, Dec 7, 2021 at 10:08 AM Fabian Paul <fp...@apache.org> wrote:
>
>> Hi all,
>>
>> Thanks Till for starting this discussion. It is great to see these
>> facts written down since they definitely caused friction in the past
>> because of different interpretations. Overall I agree with everything
>> being said in this FLIP. I was just wondering whether we can put the
>> label explaining table somewhere into our docs. FLIPs are usually only
>> snapshots for the time being and are hard to search for. It would
>> allow users to instantly determine the meaning of the used annotation.
>>
>> I am also missing in this FLIP the connection to the Deprecated
>> annotation. I think we also need to clarify how this should be used in
>> conjunction with the API stability.
>>
>> Best,
>> Fabian
>>
>> On Mon, Dec 6, 2021 at 10:03 AM Ingo Bürk <i...@ververica.com> wrote:
>> >
>> > Hi Till,
>> >
>> > seems I misunderstood it then; thanks for the clarification! And yes,
>> with
>> > that I would fully agree.
>> >
>> >
>> > Ingo
>> >
>> > On Mon, Dec 6, 2021 at 9:59 AM Till Rohrmann <trohrm...@apache.org>
>> wrote:
>> >
>> > > Hi Ingo,
>> > >
>> > > No, the added method can have a weaker stability guarantee as long as
>> the
>> > > user does not have to implement it. In order to give an example the
>> > > following extension would be ok imo:
>> > >
>> > > @Public
>> > > interface Foobar {
>> > > @Public
>> > > int foo();
>> > >
>> > > @Experimental
>> > > default ExperimentalResult bar() {
>> > > return ExperimentalResult.notSupported();
>> > > }
>> > > }
>> > >
>> > > The following extension would not be ok because here the user needs to
>> > > implement something new:
>> > >
>> > > @Public
>> > > interface Foobar {
>> > > @Public
>> > > int foo();
>> > >
>> > > @Experimental
>> > > ExperimentalResult bar();
>> > > }
>> > >
>> > > Moreover, if the user uses bar(), then he opts-in to only get
>> @Experimental
>> > > stability guarantees.
>> > >
>> > > I will add this example to the FLIP for illustrative purposes.
>> > >
>> > > Cheers,
>> > > Till
>> > >
>> > > On Fri, Dec 3, 2021 at 6:52 PM Ingo Bürk <i...@ververica.com> wrote:
>> > >
>> > > > > Would it be enough to say that for example all classes in the
>> module
>> > > > flink-java have to be annotated? What I would like to avoid is
>> having to
>> > > > annotate all classes in some internal module like flink-rpc.
>> > > >
>> > > > I don't think it is, but we certainly could restrict it to certain
>> top
>> > > > level o.a.f.xyz packages.
>> > > >
>> > > > > Extending existing classes will only be possible if you can
>> provide a
>> > > > default implementation
>> > > >
>> > > > That I'm totally fine with, but based on that sentence in the FLIP
>> if I
>> > > > have a public interface and extend it, even with a default
>> > > implementation,
>> > > > I _have_ to have this method be stable already as well, right? I
>> couldn't
>> > > > for example add an experimental method to an interface.
>> > > >
>> > > > This would also include all classes used as argument and return
>> type of
>> > > > such methods too, which seems quite restrictive.
>> > > >
>> > > >
>> > > > Best
>> > > > Ingo
>> > > >
>> > > > On Fri, Dec 3, 2021, 17:51 Till Rohrmann <trohrm...@apache.org>
>> wrote:
>> > > >
>> > > > > >That's still a much weaker requirement, though, as classes can
>> just be
>> > > > > left unannotated, which is why I prefer annotating all classes
>> > > regardless
>> > > > > of location.
>> > > > >
>> > > > > Would it be enough to say that for example all classes in the
>> module
>> > > > > flink-java have to be annotated? What I would like to avoid is
>> having
>> > > to
>> > > > > annotate all classes in some internal module like flink-rpc.
>> > > > >
>> > > > > > How would you handle e.g. extending an existing Public interface
>> > > with a
>> > > > > new method in this case, though? You'd be forced to immediately
>> make
>> > > the
>> > > > > new method Public as well, or place it somewhere else entirely,
>> which
>> > > > leads
>> > > > > to unfavorable design. I don't think we should disallow extending
>> > > classes
>> > > > > with methods of a weaker stability.
>> > > > >
>> > > > > Extending existing classes will only be possible if you can
>> provide a
>> > > > > default implementation. If the user needs to do something, then
>> it is
>> > > not
>> > > > > compatible and needs to be handled differently (e.g. by offering
>> a new
>> > > > > experimental interface that one can use). If we don't enforce
>> this,
>> > > then
>> > > > I
>> > > > > don't see how we can provide source stability guarantees.
>> > > > >
>> > > > > Cheers,
>> > > > > Till
>> > > > >
>> > > > > On Fri, Dec 3, 2021 at 5:22 PM Ingo Bürk <i...@ververica.com>
>> wrote:
>> > > > >
>> > > > > > Hi Till,
>> > > > > >
>> > > > > > > Personally, I'd be fine to say that in API modules (tbd what
>> this
>> > > is
>> > > > > > > (probably transitive closure of all APIs)) we require every
>> class
>> > > to
>> > > > be
>> > > > > > > annotated.
>> > > > > >
>> > > > > > At least we'll then need the reverse rule: no classes outside
>> *.api.*
>> > > > > > packages CAN have an API annotation (other than Internal), of
>> course
>> > > > with
>> > > > > > many existing violations that need to be accapted.
>> > > > > >
>> > > > > > That's still a much weaker requirement, though, as classes can
>> just
>> > > be
>> > > > > left
>> > > > > > unannotated, which is why I prefer annotating all classes
>> regardless
>> > > of
>> > > > > > location.
>> > > > > >
>> > > > > > > If we have cases that violate the guideline, then I think we
>> either
>> > > > > have
>> > > > > > to
>> > > > > > > remove these methods
>> > > > > >
>> > > > > > How would you handle e.g. extending an existing Public interface
>> > > with a
>> > > > > new
>> > > > > > method in this case, though? You'd be forced to immediately
>> make the
>> > > > new
>> > > > > > method Public as well, or place it somewhere else entirely,
>> which
>> > > leads
>> > > > > to
>> > > > > > unfavorable design. I don't think we should disallow extending
>> > > classes
>> > > > > with
>> > > > > > methods of a weaker stability.
>> > > > > >
>> > > > > >
>> > > > > > Best
>> > > > > > Ingo
>> > > > > >
>> > > > > > On Fri, Dec 3, 2021 at 4:53 PM Till Rohrmann <
>> trohrm...@apache.org>
>> > > > > wrote:
>> > > > > >
>> > > > > > > Hi Ingo, thanks a lot for your thoughts and the work you've
>> spent
>> > > on
>> > > > > this
>> > > > > > > topic already.
>> > > > > > >
>> > > > > > > Personally, I'd be fine to say that in API modules (tbd what
>> this
>> > > is
>> > > > > > > (probably transitive closure of all APIs)) we require every
>> class
>> > > to
>> > > > be
>> > > > > > > annotated.
>> > > > > > >
>> > > > > > > For sake of clarity and having clear rules, this should then
>> also
>> > > > apply
>> > > > > > to
>> > > > > > > nested types. As you've said this would have some additional
>> > > benefits
>> > > > > > when
>> > > > > > > doing refactorings and seems to be actually required by
>> japicmp.
>> > > > > > >
>> > > > > > > >This seems to be quite incompatible with the current
>> > > interpretation
>> > > > in
>> > > > > > the
>> > > > > > > code base, and it would prevent valid (and in-use) use cases
>> like
>> > > > > marking
>> > > > > > > e.g. a single method experimental (or even internal) in an
>> > > otherwise
>> > > > > > public
>> > > > > > > (evolving) API.
>> > > > > > >
>> > > > > > > If we have cases that violate the guideline, then I think we
>> either
>> > > > > have
>> > > > > > to
>> > > > > > > remove these methods or adjust their stability guarantees
>> (time of
>> > > > > > > reckoning ;-). Otherwise, we won't be able to give stability
>> > > > > guarantees,
>> > > > > > I
>> > > > > > > fear.
>> > > > > > >
>> > > > > > > Cheers,
>> > > > > > > Till
>> > > > > > >
>> > > > > > > On Fri, Dec 3, 2021 at 4:23 PM Ingo Bürk <i...@ververica.com>
>> > > wrote:
>> > > > > > >
>> > > > > > > > Hi Till,
>> > > > > > > >
>> > > > > > > > Thanks for starting this discussion, and as you noticed, I
>> qutie
>> > > > care
>> > > > > > for
>> > > > > > > > it as well. :-)
>> > > > > > > >
>> > > > > > > > When implementing the ArchUnit rules, I noticed that
>> > > project-wide,
>> > > > > > > > different teams / modules use and understand the API
>> annotations
>> > > in
>> > > > > > > > different ways. Therefore, I think it is important to both
>> get
>> > > > > clarity
>> > > > > > on
>> > > > > > > > the meaning of these annotations (= your FLIP), but also on
>> how
>> > > to
>> > > > > > > actually
>> > > > > > > > use them.
>> > > > > > > >
>> > > > > > > > One thing I would like to bring up for discussion is
>> requiring an
>> > > > API
>> > > > > > > > annotation on every class. This seems to be a controversial
>> > > > > suggestion,
>> > > > > > > > though personally I think it would allow our tooling do its
>> job
>> > > > best.
>> > > > > > An
>> > > > > > > > obvious downside is "it's annoying", but so is the current
>> > > > > requirement
>> > > > > > of
>> > > > > > > > having JavaDocs on every class (including tests). If this
>> were to
>> > > > > ever
>> > > > > > be
>> > > > > > > > considered, this should be implemented (also) in Checkstyle,
>> > > > though,
>> > > > > to
>> > > > > > > > have it as immediate IDE feedback, which IMO would also
>> mostly
>> > > > > > eliminate
>> > > > > > > > this downside. The other downside is that it encourages
>> throwing
>> > > > > > > > Public(Evolving) around, but that would really only make an
>> > > > existing
>> > > > > > > > invisible problem actually visible; ultimately, I think
>> this is
>> > > > > > something
>> > > > > > > > that committers just need to consider.
>> > > > > > > > It would be nice to only require this in *.api.* packages,
>> but
>> > > > that's
>> > > > > > > just
>> > > > > > > > not the world we live in, nor will it be anytime soon;
>> however,
>> > > it
>> > > > > > would
>> > > > > > > > allow us to make sure that new non-internal APIs are only
>> added
>> > > in
>> > > > > > > *.api.*
>> > > > > > > > packages, or that no public APIs are added in *.internal.*
>> > > > packages.
>> > > > > > > >
>> > > > > > > > Another aspect that came up both when implementing the
>> rules and
>> > > > > again
>> > > > > > > in a
>> > > > > > > > discussion is whether (or when) nested types (classes,
>> > > interfaces –
>> > > > > > > > not members) need to be annotated explicitly. My
>> understanding[1]
>> > > > is
>> > > > > > that
>> > > > > > > > this is actually needed for japicmp to work correctly.
>> Besides
>> > > > that,
>> > > > > I
>> > > > > > > > think doing so adds benefits such as making the breaking
>> change
>> > > > > visible
>> > > > > > > > when promoting a nested type to a top-level type, which
>> would
>> > > > likely
>> > > > > go
>> > > > > > > > unquestioned if the type is not annotated but "inherited"
>> its API
>> > > > > > > > stability, losing it in the process.
>> > > > > > > >
>> > > > > > > > Now to actually comment on your FLIP:
>> > > > > > > >
>> > > > > > > > > In order to determine the stability guarantee a given
>> > > > > class/interface
>> > > > > > > can
>> > > > > > > > provide, we have to take a look at all methods a user has to
>> > > > > implement
>> > > > > > > and
>> > > > > > > > use for using the given class/interface. The weakest
>> guarantee of
>> > > > > these
>> > > > > > > > methods specifies the guarantee we can give for the
>> containing
>> > > > > > > > class/interface.
>> > > > > > > >
>> > > > > > > > This seems to be quite incompatible with the current
>> > > interpretation
>> > > > > in
>> > > > > > > the
>> > > > > > > > code base, and it would prevent valid (and in-use) use
>> cases like
>> > > > > > marking
>> > > > > > > > e.g. a single method experimental (or even internal) in an
>> > > > otherwise
>> > > > > > > public
>> > > > > > > > (evolving) API.
>> > > > > > > >
>> > > > > > > > [1]
>> > > > > https://github.com/apache/flink/pull/17133#issuecomment-925738694
>> > > > > > > >
>> > > > > > > >
>> > > > > > > > Best
>> > > > > > > > Ingo
>> > > > > > > >
>> > > > > > > > On Thu, Dec 2, 2021 at 3:48 PM Till Rohrmann <
>> > > trohrm...@apache.org
>> > > > >
>> > > > > > > wrote:
>> > > > > > > >
>> > > > > > > > > Hi everyone,
>> > > > > > > > >
>> > > > > > > > > I would like to start a discussion about what kind of API
>> > > > stability
>> > > > > > > > > guarantees we want to provide to our users. The Flink
>> community
>> > > > > > already
>> > > > > > > > > agreed on some stability guarantees, but these guarantees
>> were
>> > > > only
>> > > > > > > > > communicated via the ML and not properly documented [2].
>> > > > Moreover,
>> > > > > > I've
>> > > > > > > > > seen more and more complaints about breaking changes (some
>> > > > rightful
>> > > > > > and
>> > > > > > > > > others not) on the ML recently [3] because we rarely mark
>> our
>> > > > APIs
>> > > > > as
>> > > > > > > > > stable. This motivated this FLIP.
>> > > > > > > > >
>> > > > > > > > > The proposal first concentrates on source API stability
>> > > > guarantees.
>> > > > > > > > Binary
>> > > > > > > > > stability guarantees are explicitly left out for a
>> follow-up
>> > > > > > > discussion.
>> > > > > > > > >
>> > > > > > > > > In essence, the proposal follows our current stability
>> > > guarantees
>> > > > > > while
>> > > > > > > > > updating the guarantees for @PublicEvolving that we are
>> already
>> > > > > > > providing
>> > > > > > > > > w/o having stated them. Moreover, this FLIP proposes some
>> > > > > guidelines
>> > > > > > > for
>> > > > > > > > > determining the stability guarantees for an API object,
>> how to
>> > > > > evolve
>> > > > > > > > them
>> > > > > > > > > and some additional requirements for the return and
>> parameter
>> > > > types
>> > > > > > of
>> > > > > > > > > methods.
>> > > > > > > > >
>> > > > > > > > > All in all, I hope that this proposal is more reflecting
>> our
>> > > > > current
>> > > > > > > > > understanding of stability guarantees than being
>> controversial.
>> > > > One
>> > > > > > of
>> > > > > > > > the
>> > > > > > > > > outcomes of this FLIP should be to properly document these
>> > > > > guarantees
>> > > > > > > so
>> > > > > > > > > that it is easily discoverable and understandable for our
>> > > users.
>> > > > > > > > Moreover,
>> > > > > > > > > I hope that we can provide more stable APIs our users can
>> rely
>> > > > and
>> > > > > > > build
>> > > > > > > > > upon.
>> > > > > > > > >
>> > > > > > > > > There will be a follow-up FLIP discussing the problem of
>> how to
>> > > > > make
>> > > > > > > sure
>> > > > > > > > > that APIs become stable over time.
>> > > > > > > > >
>> > > > > > > > > Looking forward to your feedback.
>> > > > > > > > >
>> > > > > > > > > [1] https://cwiki.apache.org/confluence/x/IJeqCw
>> > > > > > > > > [2]
>> > > > > https://lists.apache.org/thread/5jm25783oq5svyk7rr8g1gly2ooxqhjr
>> > > > > > > > > [3]
>> > > > > https://lists.apache.org/thread/kzhfc3t6omzo2kyo8zj9yxoh8twq5fzr
>> > > > > > > > >
>> > > > > > > > > Cheers,
>> > > > > > > > > Till
>> > > > > > > > >
>> > > > > > > >
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>>
>
