tez-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bikas Saha <bi...@hortonworks.com>
Subject RE: api compatibility within a minor release
Date Thu, 20 Aug 2015 20:47:32 GMT
So what you would ideally like is a DAGClient.getVertexTaskStatus() (because internally DAGClient
forks out to either the AM or the Timeline server behind the scenes so you don’t have to
worry about it).

If so, please open a jira for that and we can put that on the roadmap.

-----Original Message-----
From: Chris K Wensel [mailto:chris@wensel.net] 
Sent: Thursday, August 20, 2015 12:02 PM
To: dev@tez.apache.org
Subject: Re: api compatibility within a minor release

hey guys

we recognize we are touching an internal api. 

and I think we are fine forcing our users to move to 0.6.2 due to the change in our minor
release since 0.6.2 resolves issues our users and sub-projects need to be in production. actually,
this is what we are doing in 3.0.2.

the api in question is the constructor in DAGClientTimelineImpl, no backward compatible constructor
was provided. was a risk we took, and lost.

we sub-class DAGClientTimelineImpl in order to leverage existing code for fetching data from
the timeline server instead of copying or re-implementing it — a reasonable practice.

we do this because there is no method on DAGClient for retrieving TaskStatus objects, but
the data is in the timeline server. we require task level data in order to retain parity with
the MapReduce implementation. 

once we moved on, we never filed an issue for the missing method (getVertexTaskStatus()).

in practice going to timeline server during runtime is a bit unsatisfactory since only started
and completed events are stored in timeline server, so we cannot access intermediate counters
as they progress. but going there eventually is required once the AM is shutdown and there
are no status access (thus DAGClient falls back to timeline server, which we inherit by sub-classing,
for the remaining methods).

ckw


> On Aug 20, 2015, at 11:29 AM, Bikas Saha <bikas@hortonworks.com> wrote:
> 
> It would be good to understand why that private method is being used. It was marked private
because we did not want it to be used. So changing it to limited private kind of defeats the
purpose, right?
> 
> If we can understand what the use case is then we can try to make it work without using
private methods.
> 
> To be clear, this should not be considered a breaking change in the API since its not
part of the documented API.
> 
> -----Original Message-----
> From: Andre Kelpe [mailto:akelpe@concurrentinc.com]
> Sent: Thursday, August 20, 2015 2:11 AM
> To: dev@tez.apache.org
> Subject: Re: api compatibility within a minor release
> 
> Thanks for the answer. We have a work-around for now. I am going to make that inventory
and submit a patch that changes the annotations from @Private to @LimitedPrivate.
> 
> From a semantic versioning point of view, I would still expect no breaking changes in
a bug fix release, but if the Tez community at large can work with that, we have to accept
that, I guess.
> 
> - André
> 
> On Tue, Aug 18, 2015 at 8:34 PM, Hitesh Shah <hitesh@apache.org> wrote:
> 
>> Hello Andre,
>> 
>> For the most part, @Private is considered internal implementation and 
>> subject to change at any point. In this case, even more so as it is 
>> an *Impl class.
>> 
>> What we can do is try the following:
>>    - Look at all the various @Private classes being used by Cascading.
>>    - See which ones should not be used at all and which ones can be 
>> considered to be a @LimitedPrivate for Cascading.
>>    - For LimitedPrivate apis, we would then try to be a bit more 
>> careful with respect to changing/breaking these APIs. I would 
>> probably not say that they have will have the same guarantees as 
>> @Public/@Stable but we can work with the Cascading community to 
>> handle changes to these APIs in a workable manner on an ongoing basis.
>> 
>> As for this API in question, for the short term fix, I guess a simple 
>> approach might be to introduce a backward compatible API ( I believe 
>> one which does not need the timeout param passed in )? Would you mind 
>> filing a jira and hopefully provide a patch too?
>> 
>> thanks
>> — Hitesh
>> 
>> On Aug 18, 2015, at 8:01 AM, Andre Kelpe <akelpe@concurrentinc.com> wrote:
>> 
>>> Hi,
>>> 
>>> I have found a small API incompatibility in the Tez 0.6.2 release. 
>>> The DAGClientTimelineImpl constructor got a new parameter for 
>>> time-outs. This was not present in 0.6.1 and from a semantic 
>>> versioning point of view,
>> that
>>> is problematic. I know that the class is marked with the @Private 
>>> annotation, but it would be great if such incompatible things aren't 
>>> introduced in a bug-fix release. It would be easier for downsteam
>> projects,
>>> if you just added a second constructor.
>>> 
>>> Is that an oversight or are classes with the @Private annotation 
>>> subject
>> to
>>> change and dowstream projects simply have to deal with it?
>>> 
>>> Thanks!
>>> 
>>> - André
>>> 
>>> --
>>> André Kelpe
>>> andre@concurrentinc.com
>>> http://concurrentinc.com
>> 
>> 
> 
> 
> --
> André Kelpe
> andre@concurrentinc.com
> http://concurrentinc.com

—
Chris K Wensel
chris@wensel.net





Mime
View raw message