Create a MapReduce-specific ReflectionUtils that handles JobConf and JobConfigurable

• Key: HADOOP-4187
• Type: Sub-task
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Sharad Agarwal
• Reporter: Tom White
• Fix Version/s: 0.20.0

Comments

[Tom White]

This is only needed for the existing mapred package. The new mapreduce package does everything in terms of Configuration (there's no JobConf), so the existing ReflectionUtils in core should suffice.

[Owen O'Malley]

Just to be clear, the problem is that core/*/utils/ReflectionUtils references mapred/*/mapred/JobConf and JobConfigurable. Clearly before the subprojects split, this needs to be removed. We need a solution for 0.20, so we can't just ignore the problem, unfortunately. I'd suggest that:
1. We make a package private mapred.ReflectionUtils that uses JobConf and JobConfigurable.
2. Make the callers in mapred call it.

[Sharad Agarwal]

We make a package private mapred.ReflectionUtils that uses JobConf and JobConfigurable.

sigh! the problem with package private is that it won't be visible to mapred.* packages.
if we don't want to add yet another public class to mapred, we can:
Option 1: duplicate code wherever required. Its just couple of lines of code so it may be OK to duplicate.

if (conf instanceof JobConf && 
        theObject instanceof JobConfigurable) {
      ((JobConfigurable)theObject).configure((JobConf) conf);
    }

Option 2: add a public static method to an existing public class in mapred where it may make sense. eg. in JobConf

On side note, one thing which might already be taken into account but I am bringing it up to be sure. This change would have potential to break user applications.
The removing of JobConf and JobConfigurable code from util.ReflectionUtils might break the applications as ReflectionUtils.newInstance and setConf are public apis; user code may be banking on to it to instantiate JobConfigurable objects.

[Tom White]

The removing of JobConf and JobConfigurable code from util.ReflectionUtils might break the applications as ReflectionUtils.newInstance and setConf are public apis; user code may be banking on to it to instantiate JobConfigurable objects.

To deal with this we could use reflection to do JobConfigurable configuration if the classes are present at runtime. This will break the compile-time dependency, which I think is sufficient for 0.20. Something like

Class<?> jobConfClass = Class.forName("org.apache.hadoop.mapred.JobConf");
Class<?> jobConfigurableClass = Class.forName("org.apache.hadoop.mapred.JobConfigurable");
 if (conf.getClass().isAssignableFrom(jobConfClass) &&
      theObject.getClass().isAssignableFrom(jobConfigurableClass)) {
  Method configureMethod = jobConfigurableClass.getMethod("configure", jobConfClass);
  configureMethod.invoke(theObject, conf);
}

We would need to be careful with classloading here.

The alternative is to make an incompatible change in 0.20. In this case I think it is OK to make mapred.ReflectionUtils public, since it will be deprecated (along with the whole mapred package) once HADOOP-1230 is finished.

[Sharad Agarwal]

Alternatives suggested by Tom make sense. What others think ?

[Doug Cutting]

+1 for Tom's proposal to use runtime class lookups.

Do we expect to complete HADOOP-1230 in 0.20?

[Sharad Agarwal]

patch for runtime class lookup.

[Sharad Agarwal]

The current patch includes a test for this in util.TestReflectionUtils which I realize may not be the ideal place. At some point, the tests will also be split between core, hdfs and mapred. So perhaps this testcase should be added to mapred in a new class mapred.TestReflectionUtils. This way it automatically gets deprecated along with the mapred package.

[Tom White]

I think we should use Configuration#getClassByName instead of Class#forName.

The current patch includes a test for this in util.TestReflectionUtils which I realize may not be the ideal place. At some point, the tests will also be split between core, hdfs and mapred. So perhaps this testcase should be added to mapred in a new class mapred.TestReflectionUtils. This way it automatically gets deprecated along with the mapred package.

I don't think we normally deprecate tests, since they aren't part of the public API. So I think what you've done is fine. When JobConf is finally removed then the test won't compile so it will be removed too. I hope that we remember to remove the code it tests too - perhaps a comment in the code would help?

[Sharad Agarwal]

Incorporated Tom's comments. Using Configuration#getClassByName instead of Class.forName.

[Devaraj Das]

Cancelling it to try hudson again

[Tom White]

+1

[Sharad Agarwal]

retrying hudson

[Hadoop QA]

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12392698/4187_v2.patch
against trunk revision 711734.

+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 warnings.

+1 Eclipse classpath. The patch retains Eclipse classpath integrity.

+1 core tests. The patch passed core unit tests.

+1 contrib tests. The patch passed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3541/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3541/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3541/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3541/console

This message is automatically generated.

[Devaraj Das]

I just committed this. Thanks, Sharad!

[Hudson]

Integrated in Hadoop-trunk #654 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/654/)
. Does a runtime lookup for JobConf/JobConfigurable, and if found, invokes the appropriate configure method. Contributed by Sharad Agarwal.