Steve Loughran commented on YARN-2423:

I'm not in a position to comment on ATS/ATSv2, except in the general RESTy 
client stuff & the tests. Low level stuff

h2. production

# {{ public ClientResponse doGettingJson(String path)}} needs to include HTTP 
error code when converting/raising exceptions. I can see the existing code 
doesn't do that, but there's no reason to copy that, is there?
# consider also including URL of server in the exceptions, as the likeliest 
cause of failure *will* be client-side config.
# this could be a good time to switch to the SLF4J logging API in the class; 
leaving old log commands alone but using the new inline expansion with 
curly-braces feature for new ones


# {{testPostEntities}} doesn't need to catch and throw an exception; throwing 
it direct stops stack traces getting lost.
# add a timeout;  a rule can do this
@Rule public Timeout testTimeout = new Timeout(30000);

# For (simple) tests that expect an exception, you can just go 
{{@Test(expected=YarnException.class)}} and have JUnit handle the checks. 
Doesn't work for content testing
# For those tests looking for string values -can you make the error strings 
constant in the production code, so if changed the tests don't break.
# we can switch to Java 7 <> constructors now

# the assertions in {{testGetEntities()}} are pretty complex. Could some of it 
be factored out, into something like {{assertEntityAt(e2, entities, 1)}}, 
{{assertEntitySize(entities, 1)}}
# If the class had a field  {{ApplicationHistoryServer ahs}}, stopping it could 
be part of teardown; no need for the try/finally clauses
# you've added krb.conf file bonded to a KDC running on port 88. Is a KDC 
needed for the tests? If so it needs to be against a miniKDC, rather than a 
local one expected to have a realm of APACHE.ORG

> TimelineClient should wrap all GET APIs to facilitate Java users
> ----------------------------------------------------------------
>                 Key: YARN-2423
>                 URL: https://issues.apache.org/jira/browse/YARN-2423
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Zhijie Shen
>            Assignee: Robert Kanter
>         Attachments: YARN-2423.004.patch, YARN-2423.005.patch, 
> YARN-2423.006.patch, YARN-2423.007.patch, YARN-2423.patch, YARN-2423.patch, 
> YARN-2423.patch
> TimelineClient provides the Java method to put timeline entities. It's also 
> good to wrap over all GET APIs (both entity and domain), and deserialize the 
> json response into Java POJO objects.

This message was sent by Atlassian JIRA

Reply via email to