|
Here's a patch showing the sort of refactoring I was talking about.
Unfortunately, the surgery was more involved than I expected: I had forgotten that TaskTracker.TaskTrackerMetrics was an inner class, which had to change. This looks like a reasonable approach. A few nits:
It would be cleaner if TaskTracker.Instrumentation were a standalone class. TaskTracker.java is already plenty big! Also, since it is public, this class needs javadoc. The 'tt' field should be private w/ accessor methods or set by the constructor. Should it be abstract? The reference to 'tt.mapTotal' should be replaced with a call to a public method, etc. Static accessor methods should be added to TaskTracker to get/set the instrumentation impl, e.g.: public static void setInstrumentation(Configuration conf, Class clazz) { ... } These should refer to a constant that names the configuration parameter name. These methods should be used in preference to using the paramter name literal. An entry should be added to hadoop-default.xml, documenting this new configuration option. Corrects nits identified by Doug.
Note – no test cases included, since it's basically a refactoring, and not a new feature. add JobTracker instrumentation, document options in hadoop-default.xml
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12386422/instrumentation-2.patch  against trunk revision 677872. +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. +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 appears to introduce 1 new Findbugs warnings. -1 release audit. The applied patch generated 213 release audit warnings (more than the trunk's current 209 warnings). +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/2904/testReport/ This message is automatically generated. this time, with Apache license at the top.
fixed findbugs, release warnings.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12386436/instrumentation-3.patch against trunk revision 678080. +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. +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 release audit. The applied patch does not increase the total number of release audit warnings. +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/2909/testReport/ This message is automatically generated. As mentioned above – no test cases included, since this is a refactoring, with no functional changes.
patch was broken by a hadoop update; about to fix.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12387040/instrumentation-4.patch against trunk revision 679930. +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. +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 release audit. The applied patch does not increase the total number of release audit warnings. +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/2961/testReport/ This message is automatically generated. I just committed this. Thanks, Ari!
Seems like errors crept into hadoop-default.xml:
The property name should be mapred.jobtracker.instrumentation. Also, the description above should be "[...] class to associate with the JobTracker." Whups, conf file part of patch was slightly wrong
Fix conf file mishap. Good catch, Vinod!
Fixes mistake in conf file. No alteration to code, no functional change, no tests.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12387481/tweak_inst.patch against trunk revision 682978. +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. +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 release audit. The applied patch does not increase the total number of release audit warnings. +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/3011/testReport/ This message is automatically generated. Integrated in Hadoop-trunk #581 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/581/
) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Major
Proposed hadoop instrumentation API
We'll probably start with TaskTrackerMetrics. We can then build an implementation of TaskTrackerInstrumentation that notifies Chukwa to pick up the task's stdout and stderr. This gives us the hooks to do log and console out harvesting for Chukwa, which lets us close
HADOOP-2206, and probably HADOOP-1199.