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