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