Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-2558

Add queue-level metrics 0.20-security branch

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.20.204.0
    • Fix Version/s: 0.20.204.0
    • Component/s: jobtracker
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      We would like to record and present the jobtracker metrics on a per-queue basis.

      1. queue-metrics-v3.patch
        21 kB
        Jeffrey Naisbitt
      2. queue-metrics-v2.patch
        27 kB
        Jeffrey Naisbitt
      3. queue-metrics.patch
        28 kB
        Jeffrey Naisbitt
      4. MAPREDUCE-2558-testfixes.patch
        3 kB
        Jeffrey Naisbitt
      5. MAPREDUCE-2558-0.20s.patch
        26 kB
        Jeffrey Naisbitt

        Activity

        Jeffrey Naisbitt created issue -
        Hide
        Jeffrey Naisbitt added a comment -

        Initial patch draft. I still need to add some tests, and I believe I am missing something with the metric registration/initialization.

        Show
        Jeffrey Naisbitt added a comment - Initial patch draft. I still need to add some tests, and I believe I am missing something with the metric registration/initialization.
        Jeffrey Naisbitt made changes -
        Field Original Value New Value
        Attachment queue-metrics.patch [ 12481226 ]
        Hide
        Jeffrey Naisbitt added a comment -

        The queue-specific metrics now appear in the jobtracker output. I still need to add some tests

        Show
        Jeffrey Naisbitt added a comment - The queue-specific metrics now appear in the jobtracker output. I still need to add some tests
        Jeffrey Naisbitt made changes -
        Attachment queue-metrics-v2.patch [ 12481241 ]
        Hide
        Luke Lu added a comment -

        Comments on patch v2:

        The queue level instrumentation class configuration is not a requirement. If you don't want to test the mechanism I'd suggest that you remove the related code (you can still keep the instrumentation methods, it's the abstract class and class loading stuff that are unnecessary).

        Looking up the queue from queue manager for metrics for every metrics update seems unnecessary as well. You can add a final Queue field in JIP and initialize it in the JIP ctor.

        You need a unique name (which also maps to a MBean name) for every queue metrics so you don't have naming conflicts when you have multiple queues. An MBean/jconsole friendly QueueInstrumentation#create would look like this:

          return ms.register("QueueMetrics,q="+ queueName, "Queue metrics",
                           new QueueMetricsSource(queueName, conf));
        
        Show
        Luke Lu added a comment - Comments on patch v2: The queue level instrumentation class configuration is not a requirement. If you don't want to test the mechanism I'd suggest that you remove the related code (you can still keep the instrumentation methods, it's the abstract class and class loading stuff that are unnecessary). Looking up the queue from queue manager for metrics for every metrics update seems unnecessary as well. You can add a final Queue field in JIP and initialize it in the JIP ctor. You need a unique name (which also maps to a MBean name) for every queue metrics so you don't have naming conflicts when you have multiple queues. An MBean/jconsole friendly QueueInstrumentation#create would look like this: return ms.register( "QueueMetrics,q=" + queueName, "Queue metrics" , new QueueMetricsSource(queueName, conf));
        Hide
        Jeffrey Naisbitt added a comment -

        With your first comment about the "queue level instrumentation class configuration", are you just saying that I should combine the QueueInstrumentation and QueueMetricSource classes, or are you just talking about removing the extra functionality for the createInstrumentation and get/setInstrumentationClass in the Queue class? (I'm not sure I understand what you mean there)

        Show
        Jeffrey Naisbitt added a comment - With your first comment about the "queue level instrumentation class configuration", are you just saying that I should combine the QueueInstrumentation and QueueMetricSource classes, or are you just talking about removing the extra functionality for the createInstrumentation and get/setInstrumentationClass in the Queue class? (I'm not sure I understand what you mean there)
        Hide
        Luke Lu added a comment -

        Your first guess is what I meant i.e., you can combine QueueInstrumentation and QueueMetricsSource and call the resulting class QueueMetrics. The introduction of the QueueInstrumentation abstract class is only necessary when you have different implementations of queue metrics, which we don't (and probably won't) have.

        Show
        Luke Lu added a comment - Your first guess is what I meant i.e., you can combine QueueInstrumentation and QueueMetricsSource and call the resulting class QueueMetrics. The introduction of the QueueInstrumentation abstract class is only necessary when you have different implementations of queue metrics, which we don't (and probably won't) have.
        Hide
        Jeffrey Naisbitt added a comment -

        Thanks. That's what I thought. I had originally created the abstract class thinking we may want to create a stub/mock queue metrics class for testing purposes. I'm fine combining the classes and go ahead and do so - unless you think we may need them for testing.

        Show
        Jeffrey Naisbitt added a comment - Thanks. That's what I thought. I had originally created the abstract class thinking we may want to create a stub/mock queue metrics class for testing purposes. I'm fine combining the classes and go ahead and do so - unless you think we may need them for testing.
        Hide
        Luke Lu added a comment -

        You can mock concrete classes the same way as long as the class and methods are not final:

        QueueMetrics metrics = Mockito.mock(QueueMetrics.class);
        
        Show
        Luke Lu added a comment - You can mock concrete classes the same way as long as the class and methods are not final: QueueMetrics metrics = Mockito.mock(QueueMetrics.class);
        Hide
        Jeffrey Naisbitt added a comment -

        Addressed Luke's comments. I'm working on adding some tests now, but I thought it would be good to get the updates posted.

        Show
        Jeffrey Naisbitt added a comment - Addressed Luke's comments. I'm working on adding some tests now, but I thought it would be good to get the updates posted.
        Jeffrey Naisbitt made changes -
        Attachment queue-metrics-v3.patch [ 12481280 ]
        Hide
        Jeffrey Naisbitt added a comment -

        Addressed Luke's comments, and added tests

        Show
        Jeffrey Naisbitt added a comment - Addressed Luke's comments, and added tests
        Jeffrey Naisbitt made changes -
        Attachment MAPREDUCE-2558-0.20s.patch [ 12481312 ]
        Jeffrey Naisbitt made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Jeffrey Naisbitt added a comment -

        This patch is for the 20-security branch, so here are the results of the manual test-patch run:
        [exec] -1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 6 new or modified tests.
        [exec]
        [exec] -1 javadoc. The javadoc tool appears to have generated 1 warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]
        [exec] -1 Eclipse classpath. The patch causes the Eclipse classpath to differ from the contents of the lib directories.
        [exec]

        The javadoc and Eclipse classpath failures are pre-existing and unrelated to these changes.

        Show
        Jeffrey Naisbitt added a comment - This patch is for the 20-security branch, so here are the results of the manual test-patch run: [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 6 new or modified tests. [exec] [exec] -1 javadoc. The javadoc tool appears to have generated 1 warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] -1 Eclipse classpath. The patch causes the Eclipse classpath to differ from the contents of the lib directories. [exec] The javadoc and Eclipse classpath failures are pre-existing and unrelated to these changes.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12481312/MAPREDUCE-2558-0.20s.patch
        against trunk revision 1130554.

        +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/hudson/job/PreCommit-MAPREDUCE-Build/340//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/12481312/MAPREDUCE-2558-0.20s.patch against trunk revision 1130554. +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/hudson/job/PreCommit-MAPREDUCE-Build/340//console This message is automatically generated.
        Hide
        Jeffrey Naisbitt added a comment -

        Again, this patch is for 0.20-security, so the test-patch failures from Hudson are not applicable.
        Additionally, Luke said it looked good to him (by email).

        Show
        Jeffrey Naisbitt added a comment - Again, this patch is for 0.20-security, so the test-patch failures from Hudson are not applicable. Additionally, Luke said it looked good to him (by email).
        Hide
        Chris Douglas added a comment -

        +1

        I committed this. Thanks, Jeff!

        Show
        Chris Douglas added a comment - +1 I committed this. Thanks, Jeff!
        Chris Douglas made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hadoop Flags [Reviewed]
        Resolution Fixed [ 1 ]
        Hide
        Jeffrey Naisbitt added a comment -

        There are some tests that use invalid queues, and a NullPointerException is occurring instead of the expected IOException. Another test uses an invalid queue for a job that it expects to pass (the invalid queue was previously ignored).

        Show
        Jeffrey Naisbitt added a comment - There are some tests that use invalid queues, and a NullPointerException is occurring instead of the expected IOException. Another test uses an invalid queue for a job that it expects to pass (the invalid queue was previously ignored).
        Jeffrey Naisbitt made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Jeffrey Naisbitt added a comment -

        Added checks for invalid queues, and fixed test that was previously expecting an invalid queue to be ignored.

        Show
        Jeffrey Naisbitt added a comment - Added checks for invalid queues, and fixed test that was previously expecting an invalid queue to be ignored.
        Jeffrey Naisbitt made changes -
        Attachment MAPREDUCE-2558-testfixes.patch [ 12481762 ]
        Jeffrey Naisbitt made changes -
        Status Reopened [ 4 ] Patch Available [ 10002 ]
        Hadoop Flags [Reviewed]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12481762/MAPREDUCE-2558-testfixes.patch
        against trunk revision 1133175.

        +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/361//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/12481762/MAPREDUCE-2558-testfixes.patch against trunk revision 1133175. +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/361//console This message is automatically generated.
        Hide
        Mahadev konar added a comment -

        I just pushed the test fix. Ideally we should have opened another jira but but its fine. thanks jeff!

        Show
        Mahadev konar added a comment - I just pushed the test fix. Ideally we should have opened another jira but but its fine. thanks jeff!
        Mahadev konar made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hadoop Flags [Reviewed]
        Resolution Fixed [ 1 ]
        Owen O'Malley made changes -
        Fix Version/s 0.20.204.0 [ 12316318 ]
        Owen O'Malley made changes -
        Fix Version/s 0.20.205.0 [ 12316391 ]
        Hide
        Owen O'Malley added a comment -

        Hadoop 0.20.204.0 was just released.

        Show
        Owen O'Malley added a comment - Hadoop 0.20.204.0 was just released.
        Owen O'Malley made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Jeffrey Naisbitt
            Reporter:
            Jeffrey Naisbitt
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development