Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.22.0
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Environment:

      linux

    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      This patch allows TaskTracker reports it's current available memory and CPU usage to JobTracker through heartbeat. The information can be used for scheduling and monitoring in the JobTracker. This patch changes the version of InterTrackerProtocal.

      Description

      The information can be used for resource aware scheduling.
      Note that this is related to MAPREDUCE-220. There the per task resource information is collected.
      This one collects the per machine information.

      1. MAPREDUCE-1218.patch
        42 kB
        Scott Chen
      2. MAPREDUCE-1218-rename.sh
        0.6 kB
        Scott Chen
      3. MAPREDUCE-1218-v2.patch
        33 kB
        Scott Chen
      4. MAPREDUCE-1218-v3.patch
        70 kB
        Scott Chen
      5. MAPREDUCE-1218-v4.patch
        72 kB
        Scott Chen
      6. MAPREDUCE-1218-v5.patch
        75 kB
        Scott Chen
      7. MAPREDUCE-1218-v6.patch
        72 kB
        Scott Chen
      8. MAPREDUCE-1218-v6.1.patch
        72 kB
        Scott Chen
      9. MAPREDUCE-1218-v6.2.patch
        73 kB
        Scott Chen

        Activity

        Hide
        Hong Tang added a comment -

        Such information may be usable across both MapReduce and HDFS, and could be a standalone real-time load collection mechanism. A logical extension here is to include io-load in the reporting too.

        Show
        Hong Tang added a comment - Such information may be usable across both MapReduce and HDFS, and could be a standalone real-time load collection mechanism. A logical extension here is to include io-load in the reporting too.
        Hide
        dhruba borthakur added a comment -

        The proposal is to include the following metrics to be reported by each TackTracker in every heartbeat:

        B1. available physical memory on this machine (in bytes)
        B2. cumulative used cpu time (for all cores) since the machine is up (in millisecond)
        B3. cpu speed on this machine (in Hz)
        B4. # of cpu cores on the machine

        I agree with Hong that the base methods that parse the /proc files to extract the above metrics be in the common subproject. That will allow both mapreduce and hdfs to use the same routines to extract useful information. On the other hand, when to extract these metrics and how best to use them (whether NN or JT) is best left to the individual subproject, isn't it? In fact, we had earlier embarked on keeping the reporting metrics reporting framework outside the JT/TT, but Vinod mentioned via MAPREDUCE-961 that it best if we can integrate them into the existing JT-TT framework, see focussed comment here:

        https://issues.apache.org/jira/browse/MAPREDUCE-961?focusedCommentId=12765422&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12765422

        @Hong: I am hoping that you agree with my proposal of keeping the routines in common subproject (so that HDFS can use it) but sending the extracted values in the JT-TT heartbeats. It makes integration with JT-TT easier and elegant.

        Show
        dhruba borthakur added a comment - The proposal is to include the following metrics to be reported by each TackTracker in every heartbeat: B1. available physical memory on this machine (in bytes) B2. cumulative used cpu time (for all cores) since the machine is up (in millisecond) B3. cpu speed on this machine (in Hz) B4. # of cpu cores on the machine I agree with Hong that the base methods that parse the /proc files to extract the above metrics be in the common subproject. That will allow both mapreduce and hdfs to use the same routines to extract useful information. On the other hand, when to extract these metrics and how best to use them (whether NN or JT) is best left to the individual subproject, isn't it? In fact, we had earlier embarked on keeping the reporting metrics reporting framework outside the JT/TT, but Vinod mentioned via MAPREDUCE-961 that it best if we can integrate them into the existing JT-TT framework, see focussed comment here: https://issues.apache.org/jira/browse/MAPREDUCE-961?focusedCommentId=12765422&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12765422 @Hong: I am hoping that you agree with my proposal of keeping the routines in common subproject (so that HDFS can use it) but sending the extracted values in the JT-TT heartbeats. It makes integration with JT-TT easier and elegant.
        Hide
        Hong Tang added a comment -

        I realized you are looking for some quick solutions. So I am fine with your suggestions.

        From a long term perspective, I think we ought to think more on the overall architecture of hadoop:

        In some cluster management systems I know of, typically load management and service discovery are structured as a standalone layer, and computation framework and storage systems like MR, HDFS would be services built on top of that. There are many advantages of such a design over the current architecture, two of which on top of head are:

        • the load information may be shared across multiple tenants that share the same resources, and they can coordinate on load balance objectives.
        • code is more modular and easier to maintain

        There are also a lot of researches on dispersing load information across nodes and using the load information wisely, particularly in situations where load information fluctuate quite frequently (so once every 20s heartbeat could be too coarse-grained). There are a series of papers in these areas from the research group where I did my Ph.D.: http://www.cs.ucsb.edu/projects/neptune/

        Show
        Hong Tang added a comment - I realized you are looking for some quick solutions. So I am fine with your suggestions. From a long term perspective, I think we ought to think more on the overall architecture of hadoop: In some cluster management systems I know of, typically load management and service discovery are structured as a standalone layer, and computation framework and storage systems like MR, HDFS would be services built on top of that. There are many advantages of such a design over the current architecture, two of which on top of head are: the load information may be shared across multiple tenants that share the same resources, and they can coordinate on load balance objectives. code is more modular and easier to maintain There are also a lot of researches on dispersing load information across nodes and using the load information wisely, particularly in situations where load information fluctuate quite frequently (so once every 20s heartbeat could be too coarse-grained). There are a series of papers in these areas from the research group where I did my Ph.D.: http://www.cs.ucsb.edu/projects/neptune/
        Hide
        dhruba borthakur added a comment -

        Thanks for your comments Hong. I agree that an external piece of monitoring software has its own set of advantages (and disadvantages).

        Based on this discussion, do we move ProcfsBasedProcessTree.java from the mapreduce subproject to the common subproject?

        Show
        dhruba borthakur added a comment - Thanks for your comments Hong. I agree that an external piece of monitoring software has its own set of advantages (and disadvantages). Based on this discussion, do we move ProcfsBasedProcessTree.java from the mapreduce subproject to the common subproject?
        Hide
        Scott Chen added a comment -

        This patch allows the TaskTracker reports its real-time CPU and memory usage.
        Here are the changes:

        1. Rename MemoryCalculatorPlugin.java -> ResourceCalculatorPlugin.java and the its subclasses
        2. Add CPU and available memory calculation in ResourceCalculatorPlugin and the subclasses.
        3. Add these new information in TaskTrackerStatus.ResourceStatus
        4. In TaskTracker.java, these information is added to TaskTrackerStatus when transmitting heartbeats

        Show
        Scott Chen added a comment - This patch allows the TaskTracker reports its real-time CPU and memory usage. Here are the changes: 1. Rename MemoryCalculatorPlugin.java -> ResourceCalculatorPlugin.java and the its subclasses 2. Add CPU and available memory calculation in ResourceCalculatorPlugin and the subclasses. 3. Add these new information in TaskTrackerStatus.ResourceStatus 4. In TaskTracker.java, these information is added to TaskTrackerStatus when transmitting heartbeats
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12426171/MAPREDUCE-1218-rename.sh
        against trunk revision 885530.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 2 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/280/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12426171/MAPREDUCE-1218-rename.sh against trunk revision 885530. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/280/console This message is automatically generated.
        Hide
        Scott Chen added a comment -

        It seems that the rename script cannot be recognized by hudson.
        So I submit the patch again without renaming.
        The renaming does not affect any function.

        Show
        Scott Chen added a comment - It seems that the rename script cannot be recognized by hudson. So I submit the patch again without renaming. The renaming does not affect any function.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12427250/MAPREDUCE-1218-v2.patch
        against trunk revision 888144.

        +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 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 failed core unit tests.

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

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/170/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/170/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/170/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/170/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12427250/MAPREDUCE-1218-v2.patch against trunk revision 888144. +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 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/170/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/170/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/170/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/170/console This message is automatically generated.
        Hide
        Scott Chen added a comment -

        The test failure is because of MAPREDUCE-1271.
        Can someone help me take a look at this patch? Thanks.

        Show
        Scott Chen added a comment - The test failure is because of MAPREDUCE-1271 . Can someone help me take a look at this patch? Thanks.
        Hide
        Vinod Kumar Vavilapalli added a comment -

        Quickly looked at the patch. Some comments:

        • Your renaming script will not work while committing too, because other classes still refer to them using the old names. You should generate a patch after renaming the classes yourselves. Classes that need to be renamed to reflect the word 'Resource' are: MemoryCalculator and all its sub-classes and TTMemoryReporting
        • Further, MemoryCalculatorPlugin.java and the its subclasses are all public. So, you still should retain the old classes, deprecate them and redirect all the old functionality to the new Resource* classes.
        • TaskTracker.java: Creation of the plugin should be taken out of initializeMemoryManagement() (+3471 through +3491) into initialize() method.
        • TTConfig.java: TT_MEMORY_CALCULATOR_PLUGIN needs to be renamed too.
        • TaskTrackerStatus.java: Why isn't cpu-usage serialized and de-serialized directly? Calculating it at the time of de-serialization from cumulativeCpuTime will yield wrong results, I think.
        • LinuxMemoryCalculatorPlugin:
          • JIFFY_LENGTH calculation code is duplicated from MAPREDUCE-1201. Should that be a blocker for this one?
          • In the main method, I didn't understand why we should sleep for 500ms. What is the reason for doing this?
          • This class has a lot of parsing and calculation code. Though I could verify them, it will be more helpful if we can write tests validating this. For this, we can write dummy /proc files ourselves and call the calculator class's methods similar to some of the TestProcfsBasedProcessTree tests, for e.g., testMemoryForProcessTree(). Can you add some similar tests here too, verifying each of the APIs?
        Show
        Vinod Kumar Vavilapalli added a comment - Quickly looked at the patch. Some comments: Your renaming script will not work while committing too, because other classes still refer to them using the old names. You should generate a patch after renaming the classes yourselves. Classes that need to be renamed to reflect the word 'Resource' are: MemoryCalculator and all its sub-classes and TTMemoryReporting Further, MemoryCalculatorPlugin.java and the its subclasses are all public. So, you still should retain the old classes, deprecate them and redirect all the old functionality to the new Resource* classes. TaskTracker.java: Creation of the plugin should be taken out of initializeMemoryManagement() (+3471 through +3491) into initialize() method. TTConfig.java: TT_MEMORY_CALCULATOR_PLUGIN needs to be renamed too. TaskTrackerStatus.java: Why isn't cpu-usage serialized and de-serialized directly? Calculating it at the time of de-serialization from cumulativeCpuTime will yield wrong results, I think. LinuxMemoryCalculatorPlugin: JIFFY_LENGTH calculation code is duplicated from MAPREDUCE-1201 . Should that be a blocker for this one? In the main method, I didn't understand why we should sleep for 500ms. What is the reason for doing this? This class has a lot of parsing and calculation code. Though I could verify them, it will be more helpful if we can write tests validating this. For this, we can write dummy /proc files ourselves and call the calculator class's methods similar to some of the TestProcfsBasedProcessTree tests, for e.g., testMemoryForProcessTree(). Can you add some similar tests here too, verifying each of the APIs?
        Hide
        Scott Chen added a comment -

        Thanks for the review.

        Your renaming script will not work while committing too, because other classes still refer to them using the old names. You should generate a patch after renaming the classes yourselves. Classes that need to be renamed to reflect the word 'Resource' are: MemoryCalculator and all its sub-classes and TTMemoryReporting

        I see. I will upload a patch that also renames the classes and methods. I will globally check 'Memory' and replace them with 'Resource' if it is necessary.

        Further, MemoryCalculatorPlugin.java and the its subclasses are all public. So, you still should retain the old classes, deprecate them and redirect all the old functionality to the new Resource* classes.

        I see. This way I don't have to rename so many things.

        TaskTracker.java: Creation of the plugin should be taken out of initializeMemoryManagement() (+3471 through +3491) into initialize() method.
        TTConfig.java: TT_MEMORY_CALCULATOR_PLUGIN needs to be renamed too.

        Will do the necessary renaming.

        TaskTrackerStatus.java: Why isn't cpu-usage serialized and de-serialized directly? Calculating it at the time of de-serialization from cumulativeCpuTime will yield wrong results, I think.

        From /proc/stat, we can only get the cumulative time. So the CPU-usage has to be calculated by differencing. And we will always need to sample twice to be able to do the differencing. I do this in the receiving side because this can save a little bandwidth (one double). But you are right. This way the CPU-usage may be affected by the nonuniform delay caused by network transmission. I will move the calculation in the transmitter side and serialize it.

        1. LinuxMemoryCalculatorPlugin:
        • JIFFY_LENGTH calculation code is duplicated from MAPREDUCE-1201. Should that be a blocker for this one?

        We should use ProcfsBasedProcessTree.JIFFY_LENGTH_IN_MILLIS here. For now I will keep the calculation code here just to make the code readable. After MAPREDUCE-1201 is committed. I will fix this part.

        • In the main method, I didn't understand why we should sleep for 500ms. What is the reason for doing this?

        This is because we can only get the cumulative CPU time from /proc/stat. To compute the CPU usage, we need to compute the difference. So we delay a while and take another sample.

        • This class has a lot of parsing and calculation code. Though I could verify them, it will be more helpful if we can write tests validating this. For this, we can write dummy /proc files ourselves and call the calculator class's methods similar to some of the TestProcfsBasedProcessTree tests, for e.g., testMemoryForProcessTree(). Can you add some similar tests here too, verifying each of the APIs?

        I see. This kind of tests will definitely be helpful. I will work on it. Thanks.

        Show
        Scott Chen added a comment - Thanks for the review. Your renaming script will not work while committing too, because other classes still refer to them using the old names. You should generate a patch after renaming the classes yourselves. Classes that need to be renamed to reflect the word 'Resource' are: MemoryCalculator and all its sub-classes and TTMemoryReporting I see. I will upload a patch that also renames the classes and methods. I will globally check 'Memory' and replace them with 'Resource' if it is necessary. Further, MemoryCalculatorPlugin.java and the its subclasses are all public. So, you still should retain the old classes, deprecate them and redirect all the old functionality to the new Resource* classes. I see. This way I don't have to rename so many things. TaskTracker.java: Creation of the plugin should be taken out of initializeMemoryManagement() (+3471 through +3491) into initialize() method. TTConfig.java: TT_MEMORY_CALCULATOR_PLUGIN needs to be renamed too. Will do the necessary renaming. TaskTrackerStatus.java: Why isn't cpu-usage serialized and de-serialized directly? Calculating it at the time of de-serialization from cumulativeCpuTime will yield wrong results, I think. From /proc/stat, we can only get the cumulative time. So the CPU-usage has to be calculated by differencing. And we will always need to sample twice to be able to do the differencing. I do this in the receiving side because this can save a little bandwidth (one double). But you are right. This way the CPU-usage may be affected by the nonuniform delay caused by network transmission. I will move the calculation in the transmitter side and serialize it. LinuxMemoryCalculatorPlugin: JIFFY_LENGTH calculation code is duplicated from MAPREDUCE-1201 . Should that be a blocker for this one? We should use ProcfsBasedProcessTree.JIFFY_LENGTH_IN_MILLIS here. For now I will keep the calculation code here just to make the code readable. After MAPREDUCE-1201 is committed. I will fix this part. In the main method, I didn't understand why we should sleep for 500ms. What is the reason for doing this? This is because we can only get the cumulative CPU time from /proc/stat. To compute the CPU usage, we need to compute the difference. So we delay a while and take another sample. This class has a lot of parsing and calculation code. Though I could verify them, it will be more helpful if we can write tests validating this. For this, we can write dummy /proc files ourselves and call the calculator class's methods similar to some of the TestProcfsBasedProcessTree tests, for e.g., testMemoryForProcessTree(). Can you add some similar tests here too, verifying each of the APIs? I see. This kind of tests will definitely be helpful. I will work on it. Thanks.
        Hide
        Scott Chen added a comment -

        Have fixed several things according to Vinod's suggestions. Major changes including
        1. Add unit tests to LinuxResourceCalculatorPlugin
        2. Deprecate the original MemoryCalculatorPlugin and LinuxMemoryCalculatorPlugin and redirect their functions
        3. Rename the class, functions and configurations by replacing "memory" with "resource"

        Show
        Scott Chen added a comment - Have fixed several things according to Vinod's suggestions. Major changes including 1. Add unit tests to LinuxResourceCalculatorPlugin 2. Deprecate the original MemoryCalculatorPlugin and LinuxMemoryCalculatorPlugin and redirect their functions 3. Rename the class, functions and configurations by replacing "memory" with "resource"
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12429100/MAPREDUCE-1218-v3.patch
        against trunk revision 894165.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 15 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 failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/351/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/351/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/351/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/351/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12429100/MAPREDUCE-1218-v3.patch against trunk revision 894165. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 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 failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/351/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/351/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/351/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/351/console This message is automatically generated.
        Hide
        Scott Chen added a comment -

        Zheng provides some suggestions on this patch. He said we should move the calculation of the CPU usage inside LinuxCalculationPlugin. I will change the patch and uploaded again.

        Show
        Scott Chen added a comment - Zheng provides some suggestions on this patch. He said we should move the calculation of the CPU usage inside LinuxCalculationPlugin. I will change the patch and uploaded again.
        Hide
        Scott Chen added a comment -

        I have moved the CPU usage calculation and added the corresponding unit tests.

        Show
        Scott Chen added a comment - I have moved the CPU usage calculation and added the corresponding unit tests.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12429493/MAPREDUCE-1218-v4.patch
        against trunk revision 896265.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 15 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/Mapreduce-Patch-h3.grid.sp2.yahoo.net/246/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/246/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/246/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/246/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12429493/MAPREDUCE-1218-v4.patch against trunk revision 896265. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 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/Mapreduce-Patch-h3.grid.sp2.yahoo.net/246/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/246/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/246/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/246/console This message is automatically generated.
        Hide
        Vinod Kumar Vavilapalli added a comment -

        Looked at that latest patch. Coming out good. Some comments, most of them are finishing touches.

        • LinuxResourceCalculatorPlugin.java
          • The method #getCpuUsage() in itself doesn't update the cpu-usage. So, if one makes two calls to this method separated by a time interval, the result won't reflect the updated cpu-usage unless calls to #getCumulativeCpuTime() are explicitly made in between. This should be fixed by calling #readProcStatFile() in this method also. The main method should be modified to call this method twice and the tests should also verify this.
          • You've changed CPU_TIME_FORMAT from "^cpu[0-9][ \t]([0-9])[ \t]([0-9])[ \t]([0-9])[ \t]." to "^cpu[ \t]([0-9])[ \t]([0-9])[ \t]([0-9])[ \t].*". I guess the earlier is correct with cpu names not having any space/tab?
          • Why even have MINIMUM_UPDATE_INTERVAL in updateCpuUsage()? If this is mainly used for making sure sampleTime is not equal to lastSampleTime, then we can do so directly and remove MINIMUM_UPDATE_INTERVAL altogether.
        • TaskTracker.java
          • To handle the deprecation of mapreduce.tasktracker.memorycalculatorplugin in TaskTracker, for memory calculations we should first try to use the class denoted by this configuration if present, otherwise only we should fall back to the new resource-calculator. To facilitate this we will also retain a deprecated TTConfig.TT_MEMORY_CALCULATOR_PLUGIN constant.
          • Nit: +661 "LOG.info(" Using MemoryCalculatorPlugin : " + resourceCalculatorPlugin);" should instead be "LOG.info(" Using ResourceCalculatorPlugin : " + resourceCalculatorPlugin);"
        • Please annotate Dummy {Resource|Memory}

          CalculatorPlugin classes as @InterfaceAudience.Private because they both are only test specific.

        • We should document mapreduce.tasktracker.resourcecalculatorplugin in mapred-default.xml and remove the documentation for mapreduce.tasktracker.memorycalculatorplugin from the same.
        • Please convert TestLinuxResourceCalculatorPlugin and TestTTResourceReporting into Junit 4 testcases.
        Show
        Vinod Kumar Vavilapalli added a comment - Looked at that latest patch. Coming out good. Some comments, most of them are finishing touches. LinuxResourceCalculatorPlugin.java The method #getCpuUsage() in itself doesn't update the cpu-usage. So, if one makes two calls to this method separated by a time interval, the result won't reflect the updated cpu-usage unless calls to #getCumulativeCpuTime() are explicitly made in between. This should be fixed by calling #readProcStatFile() in this method also. The main method should be modified to call this method twice and the tests should also verify this. You've changed CPU_TIME_FORMAT from "^cpu [0-9] [ \t] ( [0-9] )[ \t] ( [0-9] )[ \t] ( [0-9] )[ \t]. " to "^cpu[ \t] ( [0-9] )[ \t] ( [0-9] )[ \t] ( [0-9] )[ \t].*" . I guess the earlier is correct with cpu names not having any space/tab? Why even have MINIMUM_UPDATE_INTERVAL in updateCpuUsage()? If this is mainly used for making sure sampleTime is not equal to lastSampleTime, then we can do so directly and remove MINIMUM_UPDATE_INTERVAL altogether. TaskTracker.java To handle the deprecation of mapreduce.tasktracker.memorycalculatorplugin in TaskTracker, for memory calculations we should first try to use the class denoted by this configuration if present, otherwise only we should fall back to the new resource-calculator. To facilitate this we will also retain a deprecated TTConfig.TT_MEMORY_CALCULATOR_PLUGIN constant. Nit: +661 "LOG.info(" Using MemoryCalculatorPlugin : " + resourceCalculatorPlugin);" should instead be "LOG.info(" Using ResourceCalculatorPlugin : " + resourceCalculatorPlugin);" Please annotate Dummy {Resource|Memory} CalculatorPlugin classes as @InterfaceAudience.Private because they both are only test specific. We should document mapreduce.tasktracker.resourcecalculatorplugin in mapred-default.xml and remove the documentation for mapreduce.tasktracker.memorycalculatorplugin from the same. Please convert TestLinuxResourceCalculatorPlugin and TestTTResourceReporting into Junit 4 testcases.
        Hide
        Scott Chen added a comment -

        Vinod, Thank you very much for the careful review.

        • LinuxResourceCalculatorPlugin.java
          • The method #getCpuUsage() in itself doesn't update the cpu-usage. So, if one makes two calls to this method separated by a time interval, the result won't reflect the updated cpu-usage unless calls to #getCumulativeCpuTime() are explicitly made in between. This should be fixed by calling #readProcStatFile() in this method also. The main method should be modified to call this method twice and the tests should also verify this.

          I agree. getCpuUsage() should be able to be called independently. I have added readProcStatFile() in this method. In testParsingProcStatAndCpuFile(), I have called this function twice without calling getCumulativeCpuTime() to verify that it can be used alone.

          • You've changed CPU_TIME_FORMAT from "^cpu[0-9][ \t]([0-9])[ \t]([0-9])[ \t]([0-9])[ \t]." to "^cpu[ \t]([0-9])[ \t]([0-9])[ \t]([0-9])[ \t].*". I guess the earlier is correct with cpu names not having any space/tab?

          Sorry, I should have explained this when I posted the patch. Originally I parse cpu0 cpu1 ... cpuN and sum there utime, stime and ntime. But latter I have found that cpu (without a number) contains the total of these values. So I have change the pattern to just parse this one line. This change is confusing.

          • Why even have MINIMUM_UPDATE_INTERVAL in updateCpuUsage()? If this is mainly used for making sure sampleTime is not equal to lastSampleTime, then we can do so directly and remove MINIMUM_UPDATE_INTERVAL altogether.

          This is for preventing people keep calling getCumulativeCpuTime (and now getCpuUsage) too often. Then the second update will have a very short update window. This may result inaccurate values. In this case, we can just skip the update and report the old value. Since the old value is still fresh in this case, it will still be quite accurate.

        • TaskTracker.java
          • To handle the deprecation of mapreduce.tasktracker.memorycalculatorplugin in TaskTracker, for memory calculations we should first try to use the class denoted by this configuration if present, otherwise only we should fall back to the new resource-calculator. To facilitate this we will also retain a deprecated TTConfig.TT_MEMORY_CALCULATOR_PLUGIN constant.

          I agree. Now it is kept in the patch.

          • Nit: +661 "LOG.info(" Using MemoryCalculatorPlugin : " + resourceCalculatorPlugin);" should instead be "LOG.info(" Using ResourceCalculatorPlugin : " + resourceCalculatorPlugin);"

          Thank you for the careful review. It is fixed.

          • Please annotate Dummy{Resource|Memory}CalculatorPlugin classes as @InterfaceAudience.Private because they both are only test specific.

          Have done the necessary change.

          • We should document mapreduce.tasktracker.resourcecalculatorplugin in mapred-default.xml and remove the documentation for mapreduce.tasktracker.memorycalculatorplugin from the same.

          Have done the necessary change.

          • Please convert TestLinuxResourceCalculatorPlugin and TestTTResourceReporting into Junit 4 testcases.

          I have added @Test and @After tags on these test cases. If there are other changes need to be done, please let me know.

        Show
        Scott Chen added a comment - Vinod, Thank you very much for the careful review. LinuxResourceCalculatorPlugin.java The method #getCpuUsage() in itself doesn't update the cpu-usage. So, if one makes two calls to this method separated by a time interval, the result won't reflect the updated cpu-usage unless calls to #getCumulativeCpuTime() are explicitly made in between. This should be fixed by calling #readProcStatFile() in this method also. The main method should be modified to call this method twice and the tests should also verify this. I agree. getCpuUsage() should be able to be called independently. I have added readProcStatFile() in this method. In testParsingProcStatAndCpuFile(), I have called this function twice without calling getCumulativeCpuTime() to verify that it can be used alone. You've changed CPU_TIME_FORMAT from "^cpu [0-9] [ \t]( [0-9] )[ \t]( [0-9] )[ \t]( [0-9] )[ \t]." to "^cpu[ \t]( [0-9] )[ \t]( [0-9] )[ \t]( [0-9] )[ \t].*". I guess the earlier is correct with cpu names not having any space/tab? Sorry, I should have explained this when I posted the patch. Originally I parse cpu0 cpu1 ... cpuN and sum there utime, stime and ntime. But latter I have found that cpu (without a number) contains the total of these values. So I have change the pattern to just parse this one line. This change is confusing. Why even have MINIMUM_UPDATE_INTERVAL in updateCpuUsage()? If this is mainly used for making sure sampleTime is not equal to lastSampleTime, then we can do so directly and remove MINIMUM_UPDATE_INTERVAL altogether. This is for preventing people keep calling getCumulativeCpuTime (and now getCpuUsage) too often. Then the second update will have a very short update window. This may result inaccurate values. In this case, we can just skip the update and report the old value. Since the old value is still fresh in this case, it will still be quite accurate. TaskTracker.java To handle the deprecation of mapreduce.tasktracker.memorycalculatorplugin in TaskTracker, for memory calculations we should first try to use the class denoted by this configuration if present, otherwise only we should fall back to the new resource-calculator. To facilitate this we will also retain a deprecated TTConfig.TT_MEMORY_CALCULATOR_PLUGIN constant. I agree. Now it is kept in the patch. Nit: +661 "LOG.info(" Using MemoryCalculatorPlugin : " + resourceCalculatorPlugin);" should instead be "LOG.info(" Using ResourceCalculatorPlugin : " + resourceCalculatorPlugin);" Thank you for the careful review. It is fixed. Please annotate Dummy{Resource|Memory}CalculatorPlugin classes as @InterfaceAudience.Private because they both are only test specific. Have done the necessary change. We should document mapreduce.tasktracker.resourcecalculatorplugin in mapred-default.xml and remove the documentation for mapreduce.tasktracker.memorycalculatorplugin from the same. Have done the necessary change. Please convert TestLinuxResourceCalculatorPlugin and TestTTResourceReporting into Junit 4 testcases. I have added @Test and @After tags on these test cases. If there are other changes need to be done, please let me know.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12429587/MAPREDUCE-1218-v5.patch
        against trunk revision 896781.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 15 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 failed core unit tests.

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

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/362/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/362/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/362/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/362/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12429587/MAPREDUCE-1218-v5.patch against trunk revision 896781. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/362/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/362/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/362/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/362/console This message is automatically generated.
        Hide
        Scott Chen added a comment -

        Have run the failed test org.apache.hadoop.mapred.TestJobTrackerStart.testJobTrackerStartConfig on my dev box. It succeed.

        Show
        Scott Chen added a comment - Have run the failed test org.apache.hadoop.mapred.TestJobTrackerStart.testJobTrackerStartConfig on my dev box. It succeed.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12429587/MAPREDUCE-1218-v5.patch
        against trunk revision 897118.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 15 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/Mapreduce-Patch-h3.grid.sp2.yahoo.net/257/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/257/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/257/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/257/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12429587/MAPREDUCE-1218-v5.patch against trunk revision 897118. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 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/Mapreduce-Patch-h3.grid.sp2.yahoo.net/257/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/257/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/257/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/257/console This message is automatically generated.
        Hide
        Vinod Kumar Vavilapalli added a comment -

        Sorry for the delay, got tied up with other things. Gone through your latest patch. Looks good overall, only two points:

        Regarding MINIMUM_UPDATE_INTERVAL

        Then the second update will have a very short update window. This may result inaccurate values.

        I see. In that case, should we instead set it to the jiffyLengthInMillis as that is the smallest unit of measurement for cpu-time?

        To handle the deprecation of mapreduce.tasktracker.memorycalculatorplugin in TaskTracker, for memory calculations we should first try to use the class denoted by this configuration if present, otherwise only we should fall back to the new resource-calculator. To facilitate this we will also retain a deprecated TTConfig.TT_MEMORY_CALCULATOR_PLUGIN constant.

        By this I actually meant the behaviour of the TT with old configuration. Even with your latest patch, if someone has the old configuration mapreduce.tasktracker.memorycalculatorplugin, it won't be of any use at all to the TT. Instead in TaskTracker.inintializeMemoryManagement(), I think we should do something like the following:

        calculatorPlugin = null;
        if (conf.get(TTConfig.TT_MEMORY_CALCULATOR_PLUGIN)!=null) {
          calculatorPlugin = MemoryCalculatorPlugin
                    .getMemoryCalculatorPlugin(clazz, fConf);
        } else {
         calculatorPlugin = resourceCalculatorPlugin;
        }
        
        if (calculatorPlugin != null) {
         totalVirtualMemoryOnTT = calculatorPlugin.getVirtualMemorySize();
        .....
        }
        

        Also, then, we will need to deprecate TTConfig.TT_MEMORY_CALCULATOR_PLUGIN constant.

        Show
        Vinod Kumar Vavilapalli added a comment - Sorry for the delay, got tied up with other things. Gone through your latest patch. Looks good overall, only two points: Regarding MINIMUM_UPDATE_INTERVAL Then the second update will have a very short update window. This may result inaccurate values. I see. In that case, should we instead set it to the jiffyLengthInMillis as that is the smallest unit of measurement for cpu-time? To handle the deprecation of mapreduce.tasktracker.memorycalculatorplugin in TaskTracker, for memory calculations we should first try to use the class denoted by this configuration if present, otherwise only we should fall back to the new resource-calculator. To facilitate this we will also retain a deprecated TTConfig.TT_MEMORY_CALCULATOR_PLUGIN constant. By this I actually meant the behaviour of the TT with old configuration. Even with your latest patch, if someone has the old configuration mapreduce.tasktracker.memorycalculatorplugin , it won't be of any use at all to the TT. Instead in TaskTracker.inintializeMemoryManagement() , I think we should do something like the following: calculatorPlugin = null ; if (conf.get(TTConfig.TT_MEMORY_CALCULATOR_PLUGIN)!= null ) { calculatorPlugin = MemoryCalculatorPlugin .getMemoryCalculatorPlugin(clazz, fConf); } else { calculatorPlugin = resourceCalculatorPlugin; } if (calculatorPlugin != null ) { totalVirtualMemoryOnTT = calculatorPlugin.getVirtualMemorySize(); ..... } Also, then, we will need to deprecate TTConfig.TT_MEMORY_CALCULATOR_PLUGIN constant.
        Hide
        Scott Chen added a comment -

        Thank you again for the review.

        In that case, should we instead set it to the jiffyLengthInMillis as that is the smallest unit of measurement for cpu-time?

        If we use the jiffyLengthInMillis, in this period of time, the CPU time can only move 1 jiffy or not for each CPU. So for each CPU, the CPU usage will be 0% or 100%. It is not very accurate. But using jiffyLengthInMillis as the unit is definitely a good idea. I have changed the MINIMUM_UPDATE_INTERVAL to 10 * jiffyLengthInMillis.

        To handle the deprecation of mapreduce.tasktracker.memorycalculatorplugin in TaskTracker, for memory calculations we should first try to use the class denoted by this configuration if present, otherwise only we should fall back to the new resource-calculator. To facilitate this we will also retain a deprecated TTConfig.TT_MEMORY_CALCULATOR_PLUGIN constant.

        I have adapted the psuedo code you've provided in TaskTracker.java. I have found that there is a mistake in the previous patch. Previously I made MemoryCalculatorPlugin extends ResourceCalculatorPlugin and LinuxMemoryCalculatorPlugin extends LinuxResourceCalculatorPlugin. But LinuxResourceCalculatorPlugin does not extend MemoryCalculatorPlugin. So MemoryCalculatorPlugin.getMemoryCalculatorPlugin will fail because it will actually returns LinuxMemoryCalculatorPlugin which does not extends MemoryCalculatorPlugin. I have fixed the problem in this patch. The same problem for DummyMemoryCalculatorPlugin as well. I have also fixed that. I have run the deleted test TestTTMemoryReporting to be sure that they work correctly.

        Show
        Scott Chen added a comment - Thank you again for the review. In that case, should we instead set it to the jiffyLengthInMillis as that is the smallest unit of measurement for cpu-time? If we use the jiffyLengthInMillis, in this period of time, the CPU time can only move 1 jiffy or not for each CPU. So for each CPU, the CPU usage will be 0% or 100%. It is not very accurate. But using jiffyLengthInMillis as the unit is definitely a good idea. I have changed the MINIMUM_UPDATE_INTERVAL to 10 * jiffyLengthInMillis. To handle the deprecation of mapreduce.tasktracker.memorycalculatorplugin in TaskTracker, for memory calculations we should first try to use the class denoted by this configuration if present, otherwise only we should fall back to the new resource-calculator. To facilitate this we will also retain a deprecated TTConfig.TT_MEMORY_CALCULATOR_PLUGIN constant. I have adapted the psuedo code you've provided in TaskTracker.java. I have found that there is a mistake in the previous patch. Previously I made MemoryCalculatorPlugin extends ResourceCalculatorPlugin and LinuxMemoryCalculatorPlugin extends LinuxResourceCalculatorPlugin. But LinuxResourceCalculatorPlugin does not extend MemoryCalculatorPlugin. So MemoryCalculatorPlugin.getMemoryCalculatorPlugin will fail because it will actually returns LinuxMemoryCalculatorPlugin which does not extends MemoryCalculatorPlugin. I have fixed the problem in this patch. The same problem for DummyMemoryCalculatorPlugin as well. I have also fixed that. I have run the deleted test TestTTMemoryReporting to be sure that they work correctly.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12429978/MAPREDUCE-1218-v6.patch
        against trunk revision 898019.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 12 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/Mapreduce-Patch-h3.grid.sp2.yahoo.net/262/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/262/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/262/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/262/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12429978/MAPREDUCE-1218-v6.patch against trunk revision 898019. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 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/Mapreduce-Patch-h3.grid.sp2.yahoo.net/262/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/262/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/262/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/262/console This message is automatically generated.
        Hide
        Vinod Kumar Vavilapalli added a comment -

        I have found that there is a mistake in the previous patch. Previously I made MemoryCalculatorPlugin extends ResourceCalculatorPlugin and LinuxMemoryCalculatorPlugin extends LinuxResourceCalculatorPlugin. But LinuxResourceCalculatorPlugin does not extend MemoryCalculatorPlugin.

        Good catch!

        DummyMemoryCalculatorPlugin needs to be deprecated still?. Other than that, +1 for the patch.

        Once that is done, and after Hudson blesses it, can you ask someone to commit this? Thanks for being patient!

        Show
        Vinod Kumar Vavilapalli added a comment - I have found that there is a mistake in the previous patch. Previously I made MemoryCalculatorPlugin extends ResourceCalculatorPlugin and LinuxMemoryCalculatorPlugin extends LinuxResourceCalculatorPlugin. But LinuxResourceCalculatorPlugin does not extend MemoryCalculatorPlugin. Good catch! DummyMemoryCalculatorPlugin needs to be deprecated still?. Other than that, +1 for the patch. Once that is done, and after Hudson blesses it, can you ask someone to commit this? Thanks for being patient!
        Hide
        Scott Chen added a comment -

        I deprecated DummyMemoryCalculatorPlugin because it extends MemoryCalculatorPlugin which is deprecated. And DummyResourceCalculatorPlugin can do what it dose.

        Thanks a lot for all the help. You are a very good reviewer.

        Show
        Scott Chen added a comment - I deprecated DummyMemoryCalculatorPlugin because it extends MemoryCalculatorPlugin which is deprecated. And DummyResourceCalculatorPlugin can do what it dose. Thanks a lot for all the help. You are a very good reviewer.
        Hide
        dhruba borthakur added a comment -

        This does not apply cleanly on trunk anymore. can you pl repost a new version of the patch?

        Show
        dhruba borthakur added a comment - This does not apply cleanly on trunk anymore. can you pl repost a new version of the patch?
        Hide
        Scott Chen added a comment -

        @Dhruba, I have updated the patch. Let me know if this one works.

        Show
        Scott Chen added a comment - @Dhruba, I have updated the patch. Let me know if this one works.
        Hide
        Scott Chen added a comment -

        Changed the version number of InterTrackerProtocol to 30L, because this patch makes the heartbeat incompatible with the old version.

        Show
        Scott Chen added a comment - Changed the version number of InterTrackerProtocol to 30L, because this patch makes the heartbeat incompatible with the old version.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12430069/MAPREDUCE-1218-v6.2.patch
        against trunk revision 898486.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 12 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/Mapreduce-Patch-h6.grid.sp2.yahoo.net/379/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/379/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/379/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/379/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12430069/MAPREDUCE-1218-v6.2.patch against trunk revision 898486. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 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/Mapreduce-Patch-h6.grid.sp2.yahoo.net/379/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/379/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/379/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/379/console This message is automatically generated.
        Hide
        dhruba borthakur added a comment -

        I just committed this. Thanks Scott!

        Show
        dhruba borthakur added a comment - I just committed this. Thanks Scott!
        Hide
        Amar Kamat added a comment -

        Dhruba, TestTTMemoryReporting is not committed.

        Show
        Amar Kamat added a comment - Dhruba, TestTTMemoryReporting is not committed.
        Hide
        dhruba borthakur added a comment -

        TestTTMemoryReporting was a file that needed to be deleted. I deleted it now.

        Show
        dhruba borthakur added a comment - TestTTMemoryReporting was a file that needed to be deleted. I deleted it now.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #206 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/206/)
        . TaskTrackers send cpu and memory usage of
        node to JobTracker. (Scott Chen via dhruba)

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #206 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/206/ ) . TaskTrackers send cpu and memory usage of node to JobTracker. (Scott Chen via dhruba)
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #225 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/225/)

        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #225 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/225/ )

          People

          • Assignee:
            Scott Chen
            Reporter:
            Scott Chen
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development