Details

Description

MiniMRYarnCluster uses a hard coded relative path location for the MapReduce application jar. It is better to have this location as a system property so tests can pick the application jar regardless of their working directory.

Hadoop QA
added a comment - 08/Nov/11 00:41 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12502800/MAPREDUCE-3370.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 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 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/1264//testReport/
Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1264//console
This message is automatically generated.

Can you also take care of MAPREDUCE-3338 as part of this? We need to make sure no test depends on MiniMRYarnCluster.HADOOP_MAPREDUCE_CLIENT_APP_JAR_NAME because of the hard-coded version. If you agree, please also fix TestDistributedShell to not use hard-coded version.

Also, how do you plan to set the sytem property? Need some changes to pom.xml files too?

Vinod Kumar Vavilapalli
added a comment - 08/Nov/11 07:43 Can you also take care of MAPREDUCE-3338 as part of this? We need to make sure no test depends on MiniMRYarnCluster.HADOOP_MAPREDUCE_CLIENT_APP_JAR_NAME because of the hard-coded version. If you agree, please also fix TestDistributedShell to not use hard-coded version.
Also, how do you plan to set the sytem property? Need some changes to pom.xml files too?

Alejandro Abdelnur
added a comment - 08/Nov/11 17:56 Ahmed, Instead using a System property, why don't you do JAR discovery by class?
JobConf has a findContainingJar() method that if you make it public would do this for you:
The the code would be:
code
public class MiniMRYarnCluster extends MiniYARNCluster
{
public static final String APPJAR =JobConf.findContainingJar(LocalContainerLauncher.class);;
...
}
code

Ahmed Radwan
added a comment - 08/Nov/11 20:20 Thanks Vinod, I'll take a look. I have assigned MAPREDUCE-3338 to myself.
Thanks Tucu, I agree, this is a nicer way. The problem is findContainingJar(..) is not visible. We can change it to public if no one objects, not sure why it is not. I'll update the patch.

Hadoop QA
added a comment - 08/Nov/11 20:44 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12502958/MAPREDUCE-3370_rev2.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified tests.
-1 patch. The patch command could not apply the patch.
Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1273//console
This message is automatically generated.

Attaching an updated patch that also addresses the TestDistributedShell.

The JobConf.findContainingJar() doesn't work for the TestDistributedShell case because the test class is part of the app jar. Generally the ability to use JobConf.findContainigJar() will depend on how maven works, when jars are created, and if tests are in separate modules.

For this reason, I have updated the patch to first check for system properties, and if not set, find the containing jar. So no need to modify the test if we decided later to move tests into separate modules and not using the system properties.

Ahmed Radwan
added a comment - 10/Nov/11 01:11 Thanks!
Attaching an updated patch that also addresses the TestDistributedShell.
The JobConf.findContainingJar() doesn't work for the TestDistributedShell case because the test class is part of the app jar. Generally the ability to use JobConf.findContainigJar() will depend on how maven works, when jars are created, and if tests are in separate modules.
For this reason, I have updated the patch to first check for system properties, and if not set, find the containing jar. So no need to modify the test if we decided later to move tests into separate modules and not using the system properties.

Hitesh Shah
added a comment - 10/Nov/11 01:18 + <yarn.ds.jar>../hadoop-yarn-applications-distributedshell/target/hadoop-yarn-applications-distributedshell-0.24.0-SNAPSHOT.jar</yarn.ds.jar>
Would it be better to use $
{version}
instead of the hardcoded version in the pom file?

Ahmed Radwan
added a comment - 10/Nov/11 20:59 Thanks! Attaching an updated patch using version. The javac warnings are the result of using the deprecated JobConf. I don't think we have another equivalent to JobConf.findContainingJar(..).

Hadoop QA
added a comment - 11/Nov/11 08:54 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12503350/MAPREDUCE-3370-20111111-branch-0.23.txt
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 patch. The patch command could not apply the patch.
Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1295//console
This message is automatically generated.