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

Default state of queues is undefined when unspecified

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 0.21.0
    • Fix Version/s: None
    • Component/s: jobtracker
    • Labels:
      None

      Description

      Currently, if the state of a queue is not specified, it is being set to "undefined" state instead of running state.

      1. MAPREDUCE-1067-6.patch
        7 kB
        V.V.Chaitanya Krishna
      2. MAPREDUCE-1067-5.patch
        7 kB
        V.V.Chaitanya Krishna
      3. MAPREDUCE-1067-4.patch
        6 kB
        V.V.Chaitanya Krishna
      4. MAPREDUCE-1067-3.patch
        6 kB
        V.V.Chaitanya Krishna
      5. MAPREDUCE-1067-2.patch
        6 kB
        V.V.Chaitanya Krishna
      6. MAPREDUCE-1067-1.patch
        6 kB
        V.V.Chaitanya Krishna

        Activity

        V.V.Chaitanya Krishna created issue -
        V.V.Chaitanya Krishna made changes -
        Field Original Value New Value
        Link This issue is part of MAPREDUCE-28 [ MAPREDUCE-28 ]
        Hide
        V.V.Chaitanya Krishna added a comment -

        This issue is being handled as part of MAPREDUCE-28, as the test cases include the testing of this issue.

        Show
        V.V.Chaitanya Krishna added a comment - This issue is being handled as part of MAPREDUCE-28 , as the test cases include the testing of this issue.
        Hide
        rahul k singh added a comment -

        Currently there are 3("running","stopped" and "undefined") states for queue , we need to revert back to old state implementation , where in there are only 2 states , "running" and "stopped" . Queues with children , would have default state of "running".

        Show
        rahul k singh added a comment - Currently there are 3("running","stopped" and "undefined") states for queue , we need to revert back to old state implementation , where in there are only 2 states , "running" and "stopped" . Queues with children , would have default state of "running".
        Hide
        Vinod Kumar Vavilapalli added a comment -

        I originally intended "undefined" state only for a ContainerQueue during MAPREDUCE-893, but apparently, edges cases were never dealt with.

        I've no problem with two states or three states. The only issue that I think should be handled is inconsistency of states for ContainerQueue. If one doesn't specify any state for a ContainerQueue, it is assigned a 'running state'. But I am not quite sure if it is always set to 'running', even if an explicit 'stopped' state is set or changed to 'stopped' from 'running' during refresh.

        And oh, let's not do this as part of MAPREDUCE-28 and mix up issues.

        Show
        Vinod Kumar Vavilapalli added a comment - I originally intended "undefined" state only for a ContainerQueue during MAPREDUCE-893 , but apparently, edges cases were never dealt with. I've no problem with two states or three states. The only issue that I think should be handled is inconsistency of states for ContainerQueue. If one doesn't specify any state for a ContainerQueue, it is assigned a 'running state'. But I am not quite sure if it is always set to 'running', even if an explicit 'stopped' state is set or changed to 'stopped' from 'running' during refresh. And oh, let's not do this as part of MAPREDUCE-28 and mix up issues.
        Hide
        rahul k singh added a comment -

        +1 , with thoughts that ContainerQueue should handle scenarios when we try to change the state of Container queue , the state of ContainerQueue will always be "stopped".

        It should not be allowed either change during refresh or have a running state.

        +1 with the thought that it should not be handled as part of MAPREDUCE-28

        Show
        rahul k singh added a comment - +1 , with thoughts that ContainerQueue should handle scenarios when we try to change the state of Container queue , the state of ContainerQueue will always be "stopped". It should not be allowed either change during refresh or have a running state. +1 with the thought that it should not be handled as part of MAPREDUCE-28
        V.V.Chaitanya Krishna made changes -
        Link This issue is part of MAPREDUCE-28 [ MAPREDUCE-28 ]
        rahul k singh made changes -
        Priority Major [ 3 ] Blocker [ 1 ]
        Vinod Kumar Vavilapalli made changes -
        Fix Version/s 0.21.0 [ 12314045 ]
        Affects Version/s 0.21.0 [ 12314045 ]
        Component/s jobtracker [ 12312907 ]
        V.V.Chaitanya Krishna made changes -
        Assignee V.V.Chaitanya Krishna [ chaitk ]
        Hide
        V.V.Chaitanya Krishna added a comment -

        Had an offline discussion with Rahul and Vinod and came up with the following conclusions:

        • The default state of container queue is to be RUNNING.
        • Also, state of a container queue should not be allowed to change once the hierarchy is built.
        • The state UNDEFINED is to be removed and only two states (RUNNING and STOPPED) are maintained.

        Uploading patch with the above implemented.

        Show
        V.V.Chaitanya Krishna added a comment - Had an offline discussion with Rahul and Vinod and came up with the following conclusions: The default state of container queue is to be RUNNING. Also, state of a container queue should not be allowed to change once the hierarchy is built. The state UNDEFINED is to be removed and only two states (RUNNING and STOPPED) are maintained. Uploading patch with the above implemented.
        V.V.Chaitanya Krishna made changes -
        Attachment MAPREDUCE-1067-1.patch [ 12426519 ]
        Hide
        rahul k singh added a comment -

        Queue.java
        1. In setState we are not checking if the state passed is RUNNING or STOPPED.
        Also there should be check in addChild , when we add a child , we need to check if State is changed to STOPPED or not,
        if yes we should throw an error.
        This is required as the order in which <queue> tag or <state> is not defined.

        QueueConfigurationParser.java
        1 . The check for queue being a container queue is not required. As we already check this in validate method , wherein we check for
        queue tag and state tag being siblings.Can you confirm this observation?

        QueueState.java
        1. We do not need a enumMap for states.

        Show
        rahul k singh added a comment - Queue.java 1. In setState we are not checking if the state passed is RUNNING or STOPPED. Also there should be check in addChild , when we add a child , we need to check if State is changed to STOPPED or not, if yes we should throw an error. This is required as the order in which <queue> tag or <state> is not defined. QueueConfigurationParser.java 1 . The check for queue being a container queue is not required. As we already check this in validate method , wherein we check for queue tag and state tag being siblings.Can you confirm this observation? QueueState.java 1. We do not need a enumMap for states.
        Hide
        V.V.Chaitanya Krishna added a comment -

        Uploading patch with the above comments implemented.

        Show
        V.V.Chaitanya Krishna added a comment - Uploading patch with the above comments implemented.
        V.V.Chaitanya Krishna made changes -
        Attachment MAPREDUCE-1067-2.patch [ 12427299 ]
        Hide
        V.V.Chaitanya Krishna added a comment -

        missed the removal of enumMap in the previous patch.
        Uploading new patch.

        Show
        V.V.Chaitanya Krishna added a comment - missed the removal of enumMap in the previous patch. Uploading new patch.
        V.V.Chaitanya Krishna made changes -
        Attachment MAPREDUCE-1067-3.patch [ 12427311 ]
        Hide
        rahul k singh added a comment -

        Queue.java
        1. if(state == null) should be done first .
        In the same method setState ,
        replace the exception message with this :
        "is a container queue , its state always running and cannot be reset."

        2. children.add(child);
        if (QueueState.STOPPED.equals(this.state))

        { throw new IllegalStateException(this.name + " is a container queue and cannot be in STOPPED state."); }

        In the above scenario , the if check should be before we add children.I think it should be before
        children == null check , first statement in the method.
        The message above should be adding child to a queue with state as stopped.

        Also mention in comments that this check is not required if Queue object is built from the XML , this indescrepency
        might happen if this Queue is directly built.

        Show
        rahul k singh added a comment - Queue.java 1. if(state == null) should be done first . In the same method setState , replace the exception message with this : "is a container queue , its state always running and cannot be reset." 2. children.add(child); if (QueueState.STOPPED.equals(this.state)) { throw new IllegalStateException(this.name + " is a container queue and cannot be in STOPPED state."); } In the above scenario , the if check should be before we add children.I think it should be before children == null check , first statement in the method. The message above should be adding child to a queue with state as stopped. Also mention in comments that this check is not required if Queue object is built from the XML , this indescrepency might happen if this Queue is directly built.
        Hide
        V.V.Chaitanya Krishna added a comment -

        Uploading patch with the above mentioned modifications done.

        Show
        V.V.Chaitanya Krishna added a comment - Uploading patch with the above mentioned modifications done.
        V.V.Chaitanya Krishna made changes -
        Attachment MAPREDUCE-1067-4.patch [ 12427326 ]
        Hide
        rahul k singh added a comment -

        Queue.java
        1.we can use children instead of this.children , similary for this.name.

        QueueState.java
        1. comment in getState is incorrect.
        // check if state is in uppercase and then set the state.
        instead of above it should be
        //we are passing uppercase , as java returns the respective case enum object
        //so for "stopped" valueOf returns the lowercase enum "stopped" object.

        2. TestQueueManager.
        There is validation check only for child queue and acls. we need to also include a
        state check . That is validation for case where in we have state and child queue defined.

        Show
        rahul k singh added a comment - Queue.java 1.we can use children instead of this.children , similary for this.name. QueueState.java 1. comment in getState is incorrect. // check if state is in uppercase and then set the state. instead of above it should be //we are passing uppercase , as java returns the respective case enum object //so for "stopped" valueOf returns the lowercase enum "stopped" object. 2. TestQueueManager. There is validation check only for child queue and acls. we need to also include a state check . That is validation for case where in we have state and child queue defined.
        Hide
        V.V.Chaitanya Krishna added a comment -

        Uploading patch with the above mentioned changes made.

        Show
        V.V.Chaitanya Krishna added a comment - Uploading patch with the above mentioned changes made.
        V.V.Chaitanya Krishna made changes -
        Attachment MAPREDUCE-1067-5.patch [ 12427454 ]
        Hide
        rahul k singh added a comment -

        some very minor nits:
        QueueConfigurationParser.java
        1. if (queueState != null && !(children != null && children.size() > 0)) {
        queueState != null check

        QueueState.java

        Instead of comment " i //we are passing uppercase , as java returns the respective case enum object
        //so for "stopped" valueOf returns the lowercase enum "stopped" object.
        "
        We should say " We are passing uppercase , as according to javadoc for valueOf"The name must match exactly an identifier used to declare an enum constant in this type. (Extraneous whitespace characters are not permitted.)"

        --My initial comment wasnt very correct.

        Queue.java
        1. In the statement
        "throw new IllegalStateException(this.name +
        " is a container queue and cannot be set.");
        "

        It should be " is a container queue . Container queue is always in running state , and its state cannot be overridden"

        Show
        rahul k singh added a comment - some very minor nits: QueueConfigurationParser.java 1. if (queueState != null && !(children != null && children.size() > 0)) { queueState != null check QueueState.java Instead of comment " i //we are passing uppercase , as java returns the respective case enum object //so for "stopped" valueOf returns the lowercase enum "stopped" object. " We should say " We are passing uppercase , as according to javadoc for valueOf"The name must match exactly an identifier used to declare an enum constant in this type. (Extraneous whitespace characters are not permitted.)" --My initial comment wasnt very correct. Queue.java 1. In the statement "throw new IllegalStateException(this.name + " is a container queue and cannot be set."); " It should be " is a container queue . Container queue is always in running state , and its state cannot be overridden"
        Hide
        V.V.Chaitanya Krishna added a comment -

        Uploading new patch with the above comments implemented.

        Show
        V.V.Chaitanya Krishna added a comment - Uploading new patch with the above comments implemented.
        V.V.Chaitanya Krishna made changes -
        Attachment MAPREDUCE-1067-6.patch [ 12428275 ]
        V.V.Chaitanya Krishna made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        rahul k singh added a comment -

        +1 with patch

        Show
        rahul k singh added a comment - +1 with patch
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12428275/MAPREDUCE-1067-6.patch
        against trunk revision 891823.

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

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/212/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/212/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/212/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/212/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/12428275/MAPREDUCE-1067-6.patch against trunk revision 891823. +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 passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/212/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/212/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/212/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/212/console This message is automatically generated.
        Hide
        V.V.Chaitanya Krishna added a comment -

        -1 contrib tests. The patch failed contrib unit tests.

        The test failures are unrelated to this issue (ref. MAPREDUCE-1311)

        Show
        V.V.Chaitanya Krishna added a comment - -1 contrib tests. The patch failed contrib unit tests. The test failures are unrelated to this issue (ref. MAPREDUCE-1311 )
        Hide
        Hemanth Yamijala added a comment -

        I have some feedback:

        • In QueueConfigurationParser.getQueueElement, we have specifically removed the check queueState != null when appending a 'state' element to the Queue element. It seems like the check is useful for preventing errors. Is there a specific reason for doing this ? For e.g. would it not be true that there could be a leaf queue given a null State on construction ? We should either prevent it in the construction or here, no ?
        • Also, state of a container queue should not be allowed to change once the hierarchy is built.

          I think the way this works is when queue configuration is refreshed, a state change for container queues will result in exceptions being thrown when the QueueConfigurationParser parses and constructs Queue objects (using setState or addChild). This results in a RuntimeException that will be handled only by the IPC handler on the server. I think no useful information about the error will reach the admin. Can you please verify if this is the case ? If yes, I think it must be fixed to be handled more gracefully, and a better error message must be given. Same holds for first time construction of hierarchy as well.

        • testQueueState should be split into multiple test cases. Typically, it is good practice to test one condition / aspect of a unit in one unit test case. It makes test case maintenance easier. There are atleast 3 distinct tests I am seeing in this test case
        • The first part of the test testQueueState is verifying a message that doesn't seem correct. For e.g. I think the XML is semantically correct, but since a queue state is defined for a container queue, construction fails. Then why are we getting an exception whose message is "Malformed xml formation queue tag and acls tags or state tags are siblings" ? The code is incorrect if this test is passing.
        • Shouldn't there be test cases testing refresh of queue state for container queues and verify they fail ?
        • There should also be a case to test that when a queue is constructed with state stopped, and then children are added to it, an exception is thrown. It is arguable if this is the right design - i.e. we should probably have ContainerQueue as a first class citizen and then we can always prevent an invalid state for such an instance, but possibly that's a separate JIRA.
        Show
        Hemanth Yamijala added a comment - I have some feedback: In QueueConfigurationParser.getQueueElement, we have specifically removed the check queueState != null when appending a 'state' element to the Queue element. It seems like the check is useful for preventing errors. Is there a specific reason for doing this ? For e.g. would it not be true that there could be a leaf queue given a null State on construction ? We should either prevent it in the construction or here, no ? Also, state of a container queue should not be allowed to change once the hierarchy is built. I think the way this works is when queue configuration is refreshed, a state change for container queues will result in exceptions being thrown when the QueueConfigurationParser parses and constructs Queue objects (using setState or addChild). This results in a RuntimeException that will be handled only by the IPC handler on the server. I think no useful information about the error will reach the admin. Can you please verify if this is the case ? If yes, I think it must be fixed to be handled more gracefully, and a better error message must be given. Same holds for first time construction of hierarchy as well. testQueueState should be split into multiple test cases. Typically, it is good practice to test one condition / aspect of a unit in one unit test case. It makes test case maintenance easier. There are atleast 3 distinct tests I am seeing in this test case The first part of the test testQueueState is verifying a message that doesn't seem correct. For e.g. I think the XML is semantically correct, but since a queue state is defined for a container queue, construction fails. Then why are we getting an exception whose message is "Malformed xml formation queue tag and acls tags or state tags are siblings" ? The code is incorrect if this test is passing. Shouldn't there be test cases testing refresh of queue state for container queues and verify they fail ? There should also be a case to test that when a queue is constructed with state stopped, and then children are added to it, an exception is thrown. It is arguable if this is the right design - i.e. we should probably have ContainerQueue as a first class citizen and then we can always prevent an invalid state for such an instance, but possibly that's a separate JIRA.
        Hemanth Yamijala made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Hide
        Tom White added a comment -

        I suspect this may not be a blocker.

        Show
        Tom White added a comment - I suspect this may not be a blocker.
        Tom White made changes -
        Fix Version/s 0.22.0 [ 12314184 ]
        Fix Version/s 0.21.0 [ 12314045 ]
        Hide
        Miguel Ochoa added a comment -

        A message that was in an mp3 format was made for an SCL component. The perspective is in a reduction that is 470° and is sold at the Radio Shack convenience store. Additional messsages should be in message04 and message07 formats.

        Show
        Miguel Ochoa added a comment - A message that was in an mp3 format was made for an SCL component. The perspective is in a reduction that is 470° and is sold at the Radio Shack convenience store. Additional messsages should be in message04 and message07 formats.
        Hide
        Nigel Daley added a comment -

        I agree that this doesn't seem like a blocker. Removing from 0.22.

        Show
        Nigel Daley added a comment - I agree that this doesn't seem like a blocker. Removing from 0.22.
        Nigel Daley made changes -
        Fix Version/s 0.22.0 [ 12314184 ]
        Priority Blocker [ 1 ] Critical [ 2 ]
        Arun C Murthy made changes -
        Priority Critical [ 2 ] Major [ 3 ]

          People

          • Assignee:
            V.V.Chaitanya Krishna
            Reporter:
            V.V.Chaitanya Krishna
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development