Hadoop QA
added a comment - 10/Jan/12 22:14 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12510105/MAPREDUCE-3299.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 6 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in .
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1584//testReport/
Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1584//console
This message is automatically generated.

Jonathan Eagles
added a comment - 10/Jan/12 21:26 Addressed JHS and AM uri rest api from jobs/
{jobid}/attempts to jobs/{jobid}
/jobattempts. Went ahead and changed the JSON and XML element names from attempts and attempt to jobAttempts and jobAttempts to be consistent with taskAttempts and taskAttempt.

Ok, we should change both the history server and app master paths then to keep them consistent.

Thanks Thomas.

This is close to commit but for the path rename. I was about to make the change myself and then push it in, but then was concerned with me missing something. Jon, can you quickly change this so that we can get this in? Thanks!

Vinod Kumar Vavilapalli
added a comment - 10/Jan/12 18:54 Ok, we should change both the history server and app master paths then to keep them consistent.
Thanks Thomas.
This is close to commit but for the path rename. I was about to make the change myself and then push it in, but then was concerned with me missing something. Jon, can you quickly change this so that we can get this in? Thanks!

{jobid}/attempts"_ would mean (without giving off the context), and in both cases I got the reply as a list of all taskAttempts in the job. Attempts in MR context overwhelmingly mean the TaskAttempts, that is also the reason why we've been calling ApplicationAttempts explicitly like that. I'd prefer moving to jobattempts. Again I am not suggesting a change in the path, just a rename, it can still be under /jobs/{jobid}

Vinod Kumar Vavilapalli
added a comment - 10/Jan/12 04:53 +1 for fixing the duplication separately.
I asked a couple of MR people what _"/jobs/
{jobid}/attempts"_ would mean (without giving off the context), and in both cases I got the reply as a list of all taskAttempts in the job. Attempts in MR context overwhelmingly mean the TaskAttempts, that is also the reason why we've been calling ApplicationAttempts explicitly like that. I'd prefer moving to jobattempts. Again I am not suggesting a change in the path, just a rename, it can still be under /jobs/{jobid}
/ and rightly so.

Good point, Vinod. I would prefer to file a separate JIRA to make common between AM and JHS, which will better address a bigger code redundencies. I feel the same as Thomas as far as webservice path is concerned.

Jonathan Eagles
added a comment - 10/Jan/12 04:16 Good point, Vinod. I would prefer to file a separate JIRA to make common between AM and JHS, which will better address a bigger code redundencies. I feel the same as Thomas as far as webservice path is concerned.

Can you explain why its confusing? To get task attempts you go to /jobs/

{jobid}/tasks/{taskid}/attempts. The way REST apis work is generally you have a resource and then you would have everything that applies/owned by that resource. In this case the resource is a job and you are getting the job attempts. a job also has tasks, which also have attempts which is where you get /jobs/{jobid}

/tasks/

{taskid}

/attempts. There are other job things like /jobs/

{jobid}/counters/, jobs/{jobid}

/conf, would you prefer those have job in front of them also?

Note - I have no strong objects to changing, just want to understand and perhaps see if same things applies elsewhere.

Thomas Graves
added a comment - 10/Jan/12 03:08 Can you explain why its confusing? To get task attempts you go to /jobs/
{jobid}/tasks/{taskid}/attempts. The way REST apis work is generally you have a resource and then you would have everything that applies/owned by that resource. In this case the resource is a job and you are getting the job attempts. a job also has tasks, which also have attempts which is where you get /jobs/{jobid}
/tasks/
{taskid}
/attempts. There are other job things like /jobs/
{jobid}/counters/, jobs/{jobid}
/conf, would you prefer those have job in front of them also?
Note - I have no strong objects to changing, just want to understand and perhaps see if same things applies elsewhere.

Can you change the webservice path to "/jobs/
{jobid}/jobattempts"? The path "/jobs/{jobid}

/attempts" is confusing and relates to the TaskAttempts more than jobattempts.

Looks like it is possible to reuse AMAttemptInfo across AM webapp and HS webapp, if we have the "shortlogslink" in both. I actually don't know the difference between loglink and shortlogslink. In any case, I don't see the logslink in HS AMAttemptInfo being used at all.

Vinod Kumar Vavilapalli
added a comment - 10/Jan/12 02:51 Going through the patch for the final review/commit.
Looks good overall. A couple of comments:
Can you change the webservice path to "/jobs/
{jobid}/jobattempts"? The path "/jobs/{jobid}
/attempts" is confusing and relates to the TaskAttempts more than jobattempts.
Looks like it is possible to reuse AMAttemptInfo across AM webapp and HS webapp, if we have the "shortlogslink" in both. I actually don't know the difference between loglink and shortlogslink. In any case, I don't see the logslink in HS AMAttemptInfo being used at all.

It looks like there is a pre-existing bug (that I introduced) in the HS attempts code that you picked up when you copied it over. The nodeId has the wrong port. It has the http port instead of the normal node manager port (should be using getNodeManagerPort()). Would you mind fixing both of those (app master and history server) here? Otherwise we can file a separate jira for the HS stuff.

Thomas Graves
added a comment - 09/Jan/12 15:18 Hey Jon,
It looks like there is a pre-existing bug (that I introduced) in the HS attempts code that you picked up when you copied it over. The nodeId has the wrong port. It has the http port instead of the normal node manager port (should be using getNodeManagerPort()). Would you mind fixing both of those (app master and history server) here? Otherwise we can file a separate jira for the HS stuff.
Everything else looks good.
Thanks.

the webservice output for logs link is coming out funny (missing http:/):
"logsLink" : "/host.com:9999/node/containerlogs/container_1325883370639_0003_01_000001",
I think you want to add http:// in the AMAttemptInfo and remove it from the JobBlock.

there is new helper routine in AMWebServices to get the job and handle error checking - Job job = getJobFromJobIdString(jid, appCtx);

JAXBContextResolver.java - wrap line to 80 characters long

checkout the newest revision of AMAttemptInfo, it has a couple extra null checks.

Thomas Graves
added a comment - 06/Jan/12 21:06 Downloaded and tested,a couple comments:
the webservice output for logs link is coming out funny (missing http:/):
"logsLink" : "/host.com:9999/node/containerlogs/container_1325883370639_0003_01_000001",
I think you want to add http:// in the AMAttemptInfo and remove it from the JobBlock.
there is new helper routine in AMWebServices to get the job and handle error checking - Job job = getJobFromJobIdString(jid, appCtx);
JAXBContextResolver.java - wrap line to 80 characters long
checkout the newest revision of AMAttemptInfo, it has a couple extra null checks.
I'll add the api docs for this under MAPREDUCE-3549 .

Jonathan Eagles
added a comment - 05/Jan/12 23:44 A version with the web services and web services tests. The tests are failing at this point, but this can give everyone a jump start on looking at the code.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

Hadoop QA
added a comment - 05/Jan/12 19:16 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12509579/MAPREDUCE-3299.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
-1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed these unit tests:
org.apache.hadoop.yarn.server.nodemanager.containermanager.monitor.TestContainersMonitor
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1547//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1547//artifact/trunk/hadoop-mapreduce-project/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-app.html
Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1547//console
This message is automatically generated.

I think this information should also be made available via the web service call - something like /mapreduce/jobs/

{jobid}

/attempts. This is fairly simple and already implemented in the history server if you look at /hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-hs/src/main/java/org/apache/hadoop/mapreduce/v2/hs/webapp/dao/AMAttemptInfo.java, HsJobBlock, and HsWebServices. We could file a separate jira too.

Thomas Graves
added a comment - 05/Jan/12 19:01 I think this information should also be made available via the web service call - something like /mapreduce/jobs/
{jobid}
/attempts. This is fairly simple and already implemented in the history server if you look at /hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-hs/src/main/java/org/apache/hadoop/mapreduce/v2/hs/webapp/dao/AMAttemptInfo.java, HsJobBlock, and HsWebServices. We could file a separate jira too.

Siddharth Seth
added a comment - 03/Jan/12 23:10 Changes look good, except the log link on the AM table needs to be fixed. It should point to the nodemanager running the AM instead of to the AM itself. (There's an AM logs link on the nav bar)

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

Hadoop QA
added a comment - 03/Jan/12 21:41 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12509338/MAPREDUCE-3299.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in .
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1530//testReport/
Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1530//console
This message is automatically generated.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

Hadoop QA
added a comment - 15/Dec/11 23:47 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12507617/MAPREDUCE-3299.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed these unit tests:
org.apache.hadoop.mapreduce.v2.app.TestJobEndNotifier
org.apache.hadoop.mapreduce.v2.app.TestStagingCleanup
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1465//testReport/
Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1465//console
This message is automatically generated.