Yeah, I should have been precise. I was referring to the Hadoop annotations regarding visibility/stability, not java's visibility.
On Fri, Oct 28, 2016 at 10:38 AM, Ravi Prakash <[email protected]> wrote: > Thanks for the clarification. "regardless of the method > visibility/stability" confused me for a bit. > > On Fri, Oct 28, 2016 at 10:31 AM, Sangjin Lee <[email protected]> wrote: > > > No, private methods are free to change as far as the class contract is > > concerned. > > > > On Fri, Oct 28, 2016 at 10:30 AM, Ravi Prakash <[email protected]> > > wrote: > > > >> Would this mean that if there is a private method in > MyPublicStableClass, > >> changing which wouldn't break anything, could we still not change it? > >> > >> On Thu, Oct 27, 2016 at 2:55 PM, Sangjin Lee <[email protected]> wrote: > >> > >> > Resurrecting an old thread as part of the YARN bug bash (YARN-5130). > >> > > >> > At minimum, I believe we agree to the following (do let me know if > that > >> is > >> > not the case): > >> > (1) If the class is declared Public/Stable, no changes to the class > that > >> > breaks the Stable contract should be made between non-major releases > >> > **regardless > >> > of the method visibility/stability**. For example, the following would > >> > break the stability: > >> > - adding a new abstract method, whether that method is stable, > >> evolving, or > >> > even private > >> > - renaming a public method > >> > Although it may be possible to have methods with weaker > >> > stability/visibility, they still MUST not break the class contract. > >> > > >> > (2) We need to address the existing violations to ContainerStatus and > >> > NodeReport by adding a default implementation for **minor releases**. > >> > - ContainerStatus: YARN-3866 (2.8) > >> > - NodeReport: YARN-4293 (2.8) > >> > > >> > There are subsequent changes to ContainerStatus by YARN-2882 and > >> YARN-5430, > >> > but they are marked 2.9.0. Is there going to be 2.9.0? If not, then > >> these > >> > might not matter as 3.0.0 permits backward incompatible changes. > >> > > >> > Thoughts? > >> > > >> > On Tue, May 31, 2016 at 10:48 AM, Sangjin Lee <[email protected]> > wrote: > >> > > >> > > > >> > > > >> > > On Tue, May 31, 2016 at 10:10 AM, Chris Nauroth < > >> > [email protected]> > >> > > wrote: > >> > > > >> > >> I recommend that we update the compatibility guide with some text > >> that > >> > >> explicitly addresses subclassing/interface inheritance stability > for > >> > >> classes/interfaces annotated Stable. This is for our own benefit > >> too. > >> > (I > >> > >> often refer back to that doc when I'm coding a patch that might > have > >> a > >> > >> chance of being backwards-incompatible.) > >> > >> > >> > > > >> > > I agree that making this distinction helps not only users but also > the > >> > > hadoop contributors. In addition to updating the compatibility > guide, > >> how > >> > > about adding a new audience annotation for interfaces & abstract > >> classes > >> > > that spells out whether a 3rd-party is expected to extend/implement > >> it? > >> > > > >> > > For example, some interface can be Public/Stable for use but could > be > >> > > off-limits in terms of extending/implementing it, while another can > be > >> > > Public/Stable for use and allowed to be extended but with an > Evolving > >> > > stability. It requires a little design, but should helps us a great > >> deal > >> > on > >> > > both ends. My 2 cents. > >> > > > >> > > Sangjin > >> > > > >> > > > >> > >> > >> > >> --Chris Nauroth > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> On 5/31/16, 9:46 AM, "Karthik Kambatla" <[email protected]> > wrote: > >> > >> > >> > >> >Argh! Totally my bad on YARN-2882. Kept missing the changes to > >> > >> >ContainerStatus even after you pointed out. > >> > >> > > >> > >> >Filed YARN-5184 to fix this before we release it. Thanks for > >> pointing > >> > it > >> > >> >out, Steve. > >> > >> > > >> > >> >On Tue, May 31, 2016 at 6:00 AM, Steve Loughran < > >> > [email protected]> > >> > >> >wrote: > >> > >> > > >> > >> >> > >> > >> >> On 31 May 2016, at 05:44, Karthik Kambatla <[email protected] > >> > <mailto: > >> > >> >> [email protected]>> wrote: > >> > >> >> > >> > >> >> Inline. > >> > >> >> > >> > >> >> On Sat, May 28, 2016 at 11:34 AM, Sangjin Lee <[email protected] > >> > >> <mailto: > >> > >> >> [email protected]>> wrote: > >> > >> >> I think there is more to it. The InterfaceStability javadoc > >> states: > >> > >> >> Incompatible changes must not be made to classes marked as > >> > stable. > >> > >> >> > >> > >> >> And in practice, I don't think the class annotation can be > >> > considered a > >> > >> >> simple sum of method annotations. There is a notion of class > >> > >> >>compatibility > >> > >> >> distinct from method stability. One key example is interfaces > and > >> > >> >>abstract > >> > >> >> classes as in this case. The moment a new abstract method is > >> added, > >> > the > >> > >> >> class becomes incompatible as it would break all downstream > >> > subclasses > >> > >> >>or > >> > >> >> implementing classes. That's the case even if *all methods are > >> > declared > >> > >> >> stable*. Thus, adding any abstract method (no matter what their > >> > >> >> scope/stability is) should be considered in violation of the > >> stable > >> > >> >> contract of the class. > >> > >> >> > >> > >> >> Fair point. I was referring to them in the context of adding > >> > @Evolving > >> > >> >> methods to @Stable classes. Our policy states that "Classes not > >> > >> >>annotated > >> > >> >> are implicitly ³Private². Class members not annotated inherit > the > >> > >> >> annotations of the enclosing class." So, the annotation on a > >> method > >> > >> >> overrides that of the enclosing class. This seems pretty > >> reasonable > >> > to > >> > >> >>me. > >> > >> >> > >> > >> >> > >> > >> >> > >> > >> >> My code wouldn't even compile because new abstract methods were > >> added > >> > >> >>to a > >> > >> >> class tagged as stable. > >> > >> >> > >> > >> >> As far as I'm concerned, it doesn't meat the strict semantics of > >> > >> >>"stable", > >> > >> >> unless there is some nuance I'm missing. > >> > >> >> > >> > >> >> Therefore, I'm with Sangin: adding new abstract methods to an > >> > existing > >> > >> >> @Stable class breaks compatibility. Adding new non-abstract > >> methods > >> > >> >>‹fine. > >> > >> >> It would have been straightforward to add some new methods to, > say > >> > >> >> ContainerReport, which were no-ops/exception raising, but which > at > >> > >> least > >> > >> >> didn't break compilation. (though they may have broken codepaths > >> > which > >> > >> >> required the methods to act as getters/settes) > >> > >> >> > >> > >> >> Do you think there is reason to revisit this? If yes, we should > >> > update > >> > >> >> this for Hadoop 3. > >> > >> >> > >> > >> >> I'm not sure about revisiting. I'm raising the fact that changes > >> to > >> > >> >> classes marked as stable have broken code, and querying the > >> validity > >> > of > >> > >> >> such an operation within the constraints of the 2.x codebase. > >> > >> >> > >> > >> >> And I'm raising it on yarn-dev, as that's where things broke. If > >> we > >> > do > >> > >> >> want to revisit things, that'll mean a move to common-dev. > >> > >> >> > >> > >> >> > >> > >> >> > >> > >> >> Regarding interfaces and abstract classes, one future > enhancement > >> to > >> > >> the > >> > >> >> InterfaceStability annotation we could consider is formally > >> > separating > >> > >> >>the > >> > >> >> contract for users of the API and the implementers of the API. > >> They > >> > >> >>follow > >> > >> >> different rules. It could be feasible to have an interface as > >> > >> >>Public/Stable > >> > >> >> for users (anyone can use the API in a stable manner) but > Private > >> for > >> > >> >> implementers. The idea is that it is still a public interface > but > >> no > >> > >> >> third-party code should not subclass or implement it. I suspect > a > >> > fair > >> > >> >> amount of hadoop's public interface might fall into that > category. > >> > That > >> > >> >> itself is probably an incompatible change, so we might have to > >> wait > >> > >> >>until > >> > >> >> after 3.0, however. > >> > >> >> > >> > >> >> Interesting thought. Agree that we do not anticipate users > >> > sub-classing > >> > >> >> most of our Public-Stable classes. > >> > >> >> > >> > >> >> There are also classes which we do not anticipate end-users to > >> > directly > >> > >> >> use, but devs might want to sub-class. This applies to pluggable > >> > >> >>entities; > >> > >> >> e.g. SchedulingPolicy in fairscheduler. We are currently using > >> > >> >> Public-Evolving to capture this intent. > >> > >> >> > >> > >> >> Should we add a third annotation in addition to Audience and > >> > Stability > >> > >> >>to > >> > >> >> capture whether a class can be extended? Given the few classes > we > >> > >> >> anticipate being extended, this is likely lesser work. :) > >> > >> >> > >> > >> >> > >> > >> >> Some options. > >> > >> >> > >> > >> >> -add a specific @PluginPoint extension with different stability > >> > >> >> requirements.(stable, unstable, evolving). That tells > implementors > >> > how > >> > >> >> likely things are to break. > >> > >> >> > >> > >> >> -Add some interface to indicate really, really, unstable. That > >> comes > >> > up > >> > >> >> more with things like the Async FS APIs, where the discussion > >> there > >> > is > >> > >> >> about how it may change radically. > >> > >> >> > >> > >> >> Something like @Experimental could be that. That means not just > >> "can > >> > >> >> change" but "can go away" > >> > >> >> > >> > >> > >> > >> > >> > >> ------------------------------------------------------------ > >> --------- > >> > >> To unsubscribe, e-mail: [email protected] > >> > >> For additional commands, e-mail: [email protected] > >> > >> > >> > >> > >> > > > >> > > >> > > > > >
