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"
