Ofcourse, I do understand that if changing myPrivateMethod changed the behavior of myPublicMethod in MyPublicStable class, then that change would be a no-go
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] >>> > >> >>> > >> >>> > > >>> > >>> >> >> >
