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