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