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