hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sangjin Lee (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-5184) Fix up incompatible changes introduced on ContainerStatus and NodeReport
Date Tue, 01 Nov 2016 16:22:58 GMT

    [ https://issues.apache.org/jira/browse/YARN-5184?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15625867#comment-15625867
] 

Sangjin Lee commented on YARN-5184:
-----------------------------------

{quote}
What if the methods in question are used by rest of YARN code? If someone subclasses this
to provide an alternate implementation, calling these methods would lead to RTE.
{quote}

Yes, I agree that it is a very valid concern, as a runtime exception is a poor substitute
for a compile-time error. I think a mitigating circumstance here is that these classes are
normally extended only by the *canonical* implementation (protobuf impl): {{ContainerStatusPBImpl}}
for {{ContainerStatus}}, and {{NodeReportPBImpl}} for {{NodeReport}}. Thus, the real likelihood
that these classes would be extended by other classes would be pretty small, but it cannot
be ruled out. In practice, the only times these are extended might be test classes (mock classes).

In that sense, actually it could be an argument *for* not moving this change to the trunk.
By doing so, we would be extending this window of potential trouble into 3.0. Since 2.8 has
not been released, we could consider this as adding compatible default implementation methods
which will then be made abstract in 3.0. Thoughts?

{quote}
The reason I am raising this concern is to see if there is a need for one more annotation
that clarifies our intent about users extending classes to be plugged in.
{quote}

I do agree that we probably need another audience annotation for whether users may extend/implement
classes (which I also proposed in the original email thread):
{quote}
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.
{quote}

One practical difficulty I see is for test classes. One key need to extend/implement the interface
is because users need to provide a mocked test class. If we declare these interfaces are off
limits to implement/extend, then they would need to stick with pure mocking via mocking libraries.
I'm not sure how onerous that requirement would be.

cc [~stevel@apache.org] for his thoughts.

> Fix up incompatible changes introduced on ContainerStatus and NodeReport
> ------------------------------------------------------------------------
>
>                 Key: YARN-5184
>                 URL: https://issues.apache.org/jira/browse/YARN-5184
>             Project: Hadoop YARN
>          Issue Type: Bug
>          Components: api
>    Affects Versions: 2.9.0
>            Reporter: Karthik Kambatla
>            Assignee: Sangjin Lee
>            Priority: Blocker
>         Attachments: YARN-5184-branch-2.8.poc.patch, YARN-5184-branch-2.poc.patch
>
>
> YARN-2882 and YARN-5430 broke compatibility by adding abstract methods to ContainerStatus.
Since ContainerStatus is a Public-Stable class, adding abstract methods to this class breaks
any extensions. 
> To fix this, we should add default implementations to these new methods and not leave
them as abstract. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message