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