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