Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: jobtracker
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Hide
      Added support for hierarchical queues in the Map/Reduce framework with the following changes:
      - mapred-queues.xml is modified to a new XML template as mentioned in the JIRA.
      - Modified JobQueueInfo to contain a handle to child queues.
      - Added new APIs in the client to get 'root' queues, so that the entire hierarchy of queues can be iterated.
      -Added new APIs to get the child queues for a given queue .
      Show
      Added support for hierarchical queues in the Map/Reduce framework with the following changes: - mapred-queues.xml is modified to a new XML template as mentioned in the JIRA. - Modified JobQueueInfo to contain a handle to child queues. - Added new APIs in the client to get 'root' queues, so that the entire hierarchy of queues can be iterated. -Added new APIs to get the child queues for a given queue .

      Description

      MAPREDUCE-853 proposes to introduce a hierarchy of queues into the Map/Reduce framework. This JIRA is for defining changes to the configuration related to queues.

      The current format for defining a queue and its properties is as follows: mapred.queue.<queue-name>.<property-name>. For e.g. mapred.queue.<queue-name>.acl-submit-job. The reason for using this verbose format was to be able to reuse the Configuration parser in Hadoop. However, administrators currently using the queue configuration have already indicated a very strong desire for a more manageable format. Since, this becomes more unwieldy with hierarchical queues, the time may be good to introduce a new format for representing queue configuration.

      1. MAPREDUCE-861-1.patch
        114 kB
        rahul k singh
      2. MAPREDUCE-861-2.patch
        127 kB
        Sreekanth Ramakrishnan
      3. MAPREDUCE-861-3.patch
        172 kB
        rahul k singh
      4. MAPREDUCE-861-4.patch
        168 kB
        rahul k singh
      5. MAPREDUCE-861-5.patch
        166 kB
        rahul k singh
      6. MAPREDUCE-861-6.patch
        178 kB
        rahul k singh
      7. MAPREDUCE-861-7.patch
        179 kB
        rahul k singh

        Issue Links

          Activity

          Hide
          Hemanth Yamijala added a comment -

          Current state:

          • Queue configuration lives in mapred-queues.xml. It contains the list of queues, their ACLs and state. The pulling of these properties into a separate file happened in HADOOP-5396 and HADOOP-5913 in Hadoop 0.21. Scheduler specific queue properties live in capacity-scheduler.xml. They have the same format of property names and values as Hadoop configuration (like mapred-site.xml).
          • For backwards compatibility, the queue configuration is also supported in mapred-site.xml, but is deprecated.

          The considerations are:

          • To change the format of the file to be more friendly towards hierarchical queues. The file format can be based on a well defined XML schema that we can discuss and agree upon, that makes sense for nested queues. The other option for the file format was JSON. However, in internal discussions, it was decided that it is best not to introduce a new format for managing this information now. XML -> XML migration is less intrusive. Also, it is similar in format to other configuration in Hadoop.
          • We have an option to merge the contents of mapred-queues.xml and capacity-scheduler.xml. It seems good to be able to do this because then the nested hierarchy of queues will be defined in exactly one place and hence chances of users making mistakes in configuring the hierarchy is lesser. Otherwise, the need for validation tools greatly increases.
          • If we do agree to merge the contents, we may want to make scheduler specific properties be treated as a black-box for the framework. An option could be to treat it as string contents. And QueueManager could provide an API like getSchedulerProperties(queue), which returns a string.
          • For backwards compatibility, queue properties can still be defined in mapred-site.xml in the old format, but they cannot define hierarchies of queues there.

          Does this make sense overall ?

          Show
          Hemanth Yamijala added a comment - Current state: Queue configuration lives in mapred-queues.xml. It contains the list of queues, their ACLs and state. The pulling of these properties into a separate file happened in HADOOP-5396 and HADOOP-5913 in Hadoop 0.21. Scheduler specific queue properties live in capacity-scheduler.xml. They have the same format of property names and values as Hadoop configuration (like mapred-site.xml). For backwards compatibility, the queue configuration is also supported in mapred-site.xml, but is deprecated. The considerations are: To change the format of the file to be more friendly towards hierarchical queues. The file format can be based on a well defined XML schema that we can discuss and agree upon, that makes sense for nested queues. The other option for the file format was JSON. However, in internal discussions, it was decided that it is best not to introduce a new format for managing this information now. XML -> XML migration is less intrusive. Also, it is similar in format to other configuration in Hadoop. We have an option to merge the contents of mapred-queues.xml and capacity-scheduler.xml. It seems good to be able to do this because then the nested hierarchy of queues will be defined in exactly one place and hence chances of users making mistakes in configuring the hierarchy is lesser. Otherwise, the need for validation tools greatly increases. If we do agree to merge the contents, we may want to make scheduler specific properties be treated as a black-box for the framework. An option could be to treat it as string contents. And QueueManager could provide an API like getSchedulerProperties(queue), which returns a string. For backwards compatibility, queue properties can still be defined in mapred-site.xml in the old format, but they cannot define hierarchies of queues there. Does this make sense overall ?
          Hide
          rahul k singh added a comment -

          As mentioned above , we had an internal agreement that we would be going ahead with
          xml based configuration for hierarchial queues.

          In terms of how configuration would be structured for hierarchial queues , we had 2 options in mind.

          Option 1:
          ----------
          mapred-queues.xml would consist of the hierarchial queue hierarchy.

          Typical hierarchial queue configuration would look like:

          <queue>
              <name>q1</name>
              <queue>
                  <name>q1q1</name>
                  <administrators>
                          u1,u2,u3
                  </administrators>
                  <submitters>
                          u1,u2
                  </submitters>
                  <state>stop/running</state>
                  <schedulingContext>
                      <capacity></capacity>
                      <maxCapacity></maxCapacity>
                  </schedulingContext>
              </queue>
          </queue>
          

          The configuration above defines a queue "q1" and a single child "q1q1"

          <schedulingContext> tag would act as an black box kind of section for the mapred based parsers.
          The xsd definition of <schedulingContext> would be

          <xs:element name="schedulingContext" minOccurs="0" maxOccurs="unbounded">
                    <xs:complexType>
                          <xs:any/>
                    </xs:complexType>
                  </xs:element>
          

          By defining <schedulingContext> as <xs:any> we can extend this section of configuration to add any kind of
          tags to the <schedulingContext>.

          Advantage:
          1.This approach allows to have a single configuration file
          2. It is generic enough as in it allows users to declare scheduler properties the way they want.

          Disadvantage:
          1. This would result in having the parsing logic at different places , for framework level changes in framework and scheduler
          specific parsing would be done in scheduler.
          2. More cumbersome to implement .

          Option 2:
          ---------
          Same as option 1 except the definition of <schedulingContext> would change . It would have
          child tags <key> and <value> which would define the key value mappings of the various properties required
          by schedulers.

          For example:

          <queue>
              <name>q1</name>
              <queue>
                  <name>q1q1</name>
                  <administrators>
                     u1,u2,u3
                  </administrators>
                  <submitters>
                     u1,u2
                  </submitters>
                  <state>stop/running</state>
                  <schedulingContext>
                      <key>capacity</key>
                      <value></value>
                      <key>maxCapacity</key>
                      <value></value>
                  </schedulingContext>
              </queue>
          </queue>
          

          the new xsd for <schedulingContext> would look like

          <xs:element name="schedulingContext" minOccurs="0" maxOccurs="unbounded">
                    <xs:complexType>
                      <xs:sequence>
                        <xs:element name="key" minOccurs="0" maxOccurs="unbounded">
                        </xs:element>
                        </xs:element>
                        <xs:element name="value" minOccurs="0" maxOccurs="unbounded">
                        </xs:element>
                      </xs:sequence>
                    </xs:complexType>
          </xs:element>
          

          Advantage:
          1. Allows to have a single configuration file.
          2. Provides constant way to specify scheduling properties.
          3. Easier to implement and parsing logic now resides at one common place.

          Disadvantage:
          1. Doesn't allows the nested settings for scheduler properties.
          2. Assumes that scheduler properties would always be in key value format.

          Show
          rahul k singh added a comment - As mentioned above , we had an internal agreement that we would be going ahead with xml based configuration for hierarchial queues. In terms of how configuration would be structured for hierarchial queues , we had 2 options in mind. Option 1: ---------- mapred-queues.xml would consist of the hierarchial queue hierarchy. Typical hierarchial queue configuration would look like: <queue> <name> q1 </name> <queue> <name> q1q1 </name> <administrators> u1,u2,u3 </administrators> <submitters> u1,u2 </submitters> <state> stop/running </state> <schedulingContext> <capacity> </capacity> <maxCapacity> </maxCapacity> </schedulingContext> </queue> </queue> The configuration above defines a queue "q1" and a single child "q1q1" <schedulingContext> tag would act as an black box kind of section for the mapred based parsers. The xsd definition of <schedulingContext> would be <xs:element name= "schedulingContext" minOccurs= "0" maxOccurs= "unbounded" > <xs:complexType> <xs:any/> </xs:complexType> </xs:element> By defining <schedulingContext> as <xs:any> we can extend this section of configuration to add any kind of tags to the <schedulingContext>. Advantage: 1.This approach allows to have a single configuration file 2. It is generic enough as in it allows users to declare scheduler properties the way they want. Disadvantage: 1. This would result in having the parsing logic at different places , for framework level changes in framework and scheduler specific parsing would be done in scheduler. 2. More cumbersome to implement . Option 2: --------- Same as option 1 except the definition of <schedulingContext> would change . It would have child tags <key> and <value> which would define the key value mappings of the various properties required by schedulers. For example: <queue> <name> q1 </name> <queue> <name> q1q1 </name> <administrators> u1,u2,u3 </administrators> <submitters> u1,u2 </submitters> <state> stop/running </state> <schedulingContext> <key> capacity </key> <value> </value> <key> maxCapacity </key> <value> </value> </schedulingContext> </queue> </queue> the new xsd for <schedulingContext> would look like <xs:element name= "schedulingContext" minOccurs= "0" maxOccurs= "unbounded" > <xs:complexType> <xs:sequence> <xs:element name= "key" minOccurs= "0" maxOccurs= "unbounded" > </xs:element> </xs:element> <xs:element name= "value" minOccurs= "0" maxOccurs= "unbounded" > </xs:element> </xs:sequence> </xs:complexType> </xs:element> Advantage: 1. Allows to have a single configuration file. 2. Provides constant way to specify scheduling properties. 3. Easier to implement and parsing logic now resides at one common place. Disadvantage: 1. Doesn't allows the nested settings for scheduler properties. 2. Assumes that scheduler properties would always be in key value format.
          Hide
          rahul k singh added a comment -

          There is small error in the xsd mentioned above for option 2:

          <xs:element name="schedulingContext" minOccurs="0" maxOccurs="unbounded">
                    <xs:complexType>
                      <xs:sequence minOccurs="0" maxOccurs="unbounded">
                        <xs:element name="key" minOccurs="1" maxOccurs="1">
                        </xs:element>
                        <xs:element name="value" minOccurs="1" maxOccurs="1">
                        </xs:element>
                      </xs:sequence>
                    </xs:complexType>
          </xs:element>
          
          
          
          Show
          rahul k singh added a comment - There is small error in the xsd mentioned above for option 2: <xs:element name= "schedulingContext" minOccurs= "0" maxOccurs= "unbounded" > <xs:complexType> <xs:sequence minOccurs= "0" maxOccurs= "unbounded" > <xs:element name= "key" minOccurs= "1" maxOccurs= "1" > </xs:element> <xs:element name= "value" minOccurs= "1" maxOccurs= "1" > </xs:element> </xs:sequence> </xs:complexType> </xs:element>
          Hide
          rahul k singh added a comment -

          We had an offline discussion with owen and eric.
          There was an agreement in principal to use option 2 with slight modification .

          So all the configuration still remains the same except <schedulingContext> part. would change to <properties>

          Hence the new configuration would look like:

          <queue>
                  <name>queue1</name>
                  <properties>
                     <property key="capacity" value="40" />
                      <property key="maxcapacity" value="50" />
                  </properties>
                  <queue>
                      <name>subQueue1</name>
                      <acl-submit-job>alice,bob</acl-submit-job>
                      <state>running</state>
                      <properties>
                          <property key="capacity" value="40" />
                          <property key="maxcapacity" value="50" />
                      </properties>
                  </queue>
          </queue>
          
          Show
          rahul k singh added a comment - We had an offline discussion with owen and eric. There was an agreement in principal to use option 2 with slight modification . So all the configuration still remains the same except <schedulingContext> part. would change to <properties> Hence the new configuration would look like: <queue> <name> queue1 </name> <properties> <property key= "capacity" value= "40" /> <property key= "maxcapacity" value= "50" /> </properties> <queue> <name> subQueue1 </name> <acl-submit-job> alice,bob </acl-submit-job> <state> running </state> <properties> <property key= "capacity" value= "40" /> <property key= "maxcapacity" value= "50" /> </properties> </queue> </queue>
          Hide
          rahul k singh added a comment -

          the name of getQueues method in QueueManager would be changed to getLeafQueues.

          From the code point of view we see following places where things would be impacted.

          1. DynamicPriorityScheduler.java line : 80 ., line : 240
          2. JobTracker.java line : 3310
          3. TestQueueManager line: 56
          4. CapacityTaskScheduler line: 1045 , line: 1102

          Ill go ahead and change the call to getQueues to getLeafQueues. The getLeafQueues would return list of queues to which job can be submitted.

          P.S: For the first cut we are not allowing to submit the job at higher level queues. they are just the logical grouping of the leaf level queues.

          Show
          rahul k singh added a comment - the name of getQueues method in QueueManager would be changed to getLeafQueues. From the code point of view we see following places where things would be impacted. 1. DynamicPriorityScheduler.java line : 80 ., line : 240 2. JobTracker.java line : 3310 3. TestQueueManager line: 56 4. CapacityTaskScheduler line: 1045 , line: 1102 Ill go ahead and change the call to getQueues to getLeafQueues. The getLeafQueues would return list of queues to which job can be submitted. P.S: For the first cut we are not allowing to submit the job at higher level queues. they are just the logical grouping of the leaf level queues.
          Hide
          rahul k singh added a comment -

          small correction above:
          getLeafQueues is actually getLeafQueueNames().

          Some more clarification in terms of names of queue:

          1 . The name of the queues would be <parent>.<child>.<grandChild>.
          for example:

          <queue>
             <name>q</name>
              <queue>
                  <name>p</name>
              </queue>
           </queue>
          

          In the above example : There are 2 queues. "q" is a root level queue and "p" is a child of "q".
          The name of queue "q" would be "q";
          The name of queue "p" would be "q.p".
          We would always use this completely qualified name in the implementation.

          Users cannot name a queue like <queue-name>.<queue-name> as "." is used as separator.

          Show
          rahul k singh added a comment - small correction above: getLeafQueues is actually getLeafQueueNames(). Some more clarification in terms of names of queue: 1 . The name of the queues would be <parent>.<child>.<grandChild>. for example: <queue> <name> q </name> <queue> <name> p </name> </queue> </queue> In the above example : There are 2 queues. "q" is a root level queue and "p" is a child of "q". The name of queue "q" would be "q"; The name of queue "p" would be "q.p". We would always use this completely qualified name in the implementation. Users cannot name a queue like <queue-name>.<queue-name> as "." is used as separator.
          Hide
          rahul k singh added a comment -

          After discussing locally regarding separator , there was an agreement over ":" .

          Show
          rahul k singh added a comment - After discussing locally regarding separator , there was an agreement over ":" .
          Hide
          rahul k singh added a comment -

          Configuration for mapred-queues.xml has changed.
          Follows the same pattern mentioned above.

          Show
          rahul k singh added a comment - Configuration for mapred-queues.xml has changed. Follows the same pattern mentioned above.
          Hide
          Hemanth Yamijala added a comment -

          I've started taking a look at this code, and have come till reviewing the framework changes. Here are some comments:

          • I would suggest that we move out the parsing code for the new queues format into a separate class like QueueHierarchyBuilder, in order to keep the QueueManager class simple.
          • I would also suggest that we make the deprecated and new class stick to a unified interface that provides APIs like:
            Queue getRoot();
            boolean areAclsEnabled();
            

            These can be used by the QueueManager on the appropriate builder depending on whether old configuration is available or not. This way, the initialization code (like in the constructor) of QueueManager will be consistent, irrespective of where the queue configuration is.

          • Also, since we want to discard the mapred-site.xml loaded properties ASAP, we probably should parse and build the hierarchy from the constructor of the builder itself.
          • createHierarchy should be refactored to parse leaf level queues and non leaf level queues separately. I would suggest something along the lines of:
            for each queue node:
              newQueue.name = parseName()
              newQueue.properties = parseProperties()
              if (node is a leaf level queue) {
                parseLeafQueueProperties() // here we parse ACLs, state and other properties.
              } else {
                for every queue node:
                  recursively parse queue nodes.
              }
            
          • Calling getInnerQueues without calling addChild can cause a null pointer because children is null.
          • Also, the method getInnerQueues doesn't seem to be used. So, is it required ?
          • hashCode is overridden, but not equals
          • I assume the order of child elements of an XML tag will be in the order in which they are defined in the file. I think we must explicitly ensure that elements come in a fixed order. For e.g. currently, we are not enforcing an order on how tags under the 'queue' node are defined. This could lead to cases where if a 'queue' element comes before the 'name' element, we would call parse hierarchy for the child queue without the parent's name.
          • Shouldn't we check in populateProperties that the node is of type element.
          • Since getNamedItem can be null, we should check for it in populateProperties. This will allow us to throw a better exception.
          • In QueueManager.initialize, allQueues.putAll(leafQueues) seems unnecessary, since getInnerQueues already does this.
          • setSchedulerInfo and getSchedulerInfo should work on all queues, not just leaf queues.
          • The current refresh code is broken with reference to what we want to do long term. We should only change properties of leaf queues. Since we are planning to change the refresh model in MAPREDUCE-893, I think for completeness sake the refresh of the code should build the entire hierarchy, but pickup only leaf level queues and copy properties like acls and state. Currently, since the refresh code is adding to the root queues, hierarchy could be affected which is not in the scope even of MAPREDUCE-893.
          • getJobQueueInfos is calling getJobQueueInfo twice. We should avoid this. Not a bug in the patch, as it was already there, but it will help to fix it.
          • dumpConfiguration should be changed. It should be changed to the new JSON format.
          • DeprecatedQueueHierarchyBuilder should not hold on to an instance of the configuration

          Some minor comments on style:

          Show
          Hemanth Yamijala added a comment - I've started taking a look at this code, and have come till reviewing the framework changes. Here are some comments: I would suggest that we move out the parsing code for the new queues format into a separate class like QueueHierarchyBuilder, in order to keep the QueueManager class simple. I would also suggest that we make the deprecated and new class stick to a unified interface that provides APIs like: Queue getRoot(); boolean areAclsEnabled(); These can be used by the QueueManager on the appropriate builder depending on whether old configuration is available or not. This way, the initialization code (like in the constructor) of QueueManager will be consistent, irrespective of where the queue configuration is. Also, since we want to discard the mapred-site.xml loaded properties ASAP, we probably should parse and build the hierarchy from the constructor of the builder itself. createHierarchy should be refactored to parse leaf level queues and non leaf level queues separately. I would suggest something along the lines of: for each queue node: newQueue.name = parseName() newQueue.properties = parseProperties() if (node is a leaf level queue) { parseLeafQueueProperties() // here we parse ACLs, state and other properties. } else { for every queue node: recursively parse queue nodes. } Calling getInnerQueues without calling addChild can cause a null pointer because children is null. Also, the method getInnerQueues doesn't seem to be used. So, is it required ? hashCode is overridden, but not equals I assume the order of child elements of an XML tag will be in the order in which they are defined in the file. I think we must explicitly ensure that elements come in a fixed order. For e.g. currently, we are not enforcing an order on how tags under the 'queue' node are defined. This could lead to cases where if a 'queue' element comes before the 'name' element, we would call parse hierarchy for the child queue without the parent's name. Shouldn't we check in populateProperties that the node is of type element. Since getNamedItem can be null, we should check for it in populateProperties. This will allow us to throw a better exception. In QueueManager.initialize, allQueues.putAll(leafQueues) seems unnecessary, since getInnerQueues already does this. setSchedulerInfo and getSchedulerInfo should work on all queues, not just leaf queues. The current refresh code is broken with reference to what we want to do long term. We should only change properties of leaf queues. Since we are planning to change the refresh model in MAPREDUCE-893 , I think for completeness sake the refresh of the code should build the entire hierarchy, but pickup only leaf level queues and copy properties like acls and state. Currently, since the refresh code is adding to the root queues, hierarchy could be affected which is not in the scope even of MAPREDUCE-893 . getJobQueueInfos is calling getJobQueueInfo twice. We should avoid this. Not a bug in the patch, as it was already there, but it will help to fix it. dumpConfiguration should be changed. It should be changed to the new JSON format. DeprecatedQueueHierarchyBuilder should not hold on to an instance of the configuration Some minor comments on style:
          Hide
          Hemanth Yamijala added a comment -

          I've started taking a look at this code, and have come till reviewing the framework changes. Here are some comments:

          • I would suggest that we move out the parsing code for the new queues format into a separate class like QueueHierarchyBuilder, in order to keep the QueueManager class simple.
          • I would also suggest that we make the deprecated and new class stick to a unified interface that provides APIs like:
            Queue getRoot();
            boolean areAclsEnabled();
            

            These can be used by the QueueManager on the appropriate builder depending on whether old configuration is available or not. This way, the initialization code (like in the constructor) of QueueManager will be consistent, irrespective of where the queue configuration is.

          • Also, since we want to discard the mapred-site.xml loaded properties ASAP, we probably should parse and build the hierarchy from the constructor of the builder itself.
          • createHierarchy should be refactored to parse leaf level queues and non leaf level queues separately. I would suggest something along the lines of:
            for each queue node:
              newQueue.name = parseName()
              newQueue.properties = parseProperties()
              if (node is a leaf level queue) {
                parseLeafQueueProperties() // here we parse ACLs, state and other properties.
              } else {
                for every queue node:
                  recursively parse queue nodes.
              }
            
          • Calling getInnerQueues without calling addChild can cause a null pointer because children is null.
          • hashCode is overridden, but not equals
          • I assume the order of child elements of an XML tag will be in the order in which they are defined in the file. I think we must explicitly ensure that elements come in a fixed order. For e.g. currently, we are not enforcing an order on how tags under the 'queue' node are defined. This could lead to cases where if a 'queue' element comes before the 'name' element, we would call parse hierarchy for the child queue without the parent's name.
          • Shouldn't we check in populateProperties that the node is of type element.
          • Since getNamedItem can be null, we should check for it in populateProperties. This will allow us to throw a better exception.
          • In QueueManager.initialize, allQueues.putAll(leafQueues) seems unnecessary, since getInnerQueues already does this.
          • setSchedulerInfo and getSchedulerInfo should work on all queues, not just leaf queues.
          • The current refresh code is broken with reference to what we want to do long term. We should only change properties of leaf queues. Since we are planning to change the refresh model in MAPREDUCE-893, I think for completeness sake the refresh of the code should build the entire hierarchy, but pickup only leaf level queues and copy properties like acls and state. Currently, since the refresh code is adding to the root queues, hierarchy could be affected which is not in the scope even of MAPREDUCE-893.
          • getJobQueueInfos is calling getJobQueueInfo twice. We should avoid this. Not a bug in the patch, as it was already there, but it will help to fix it.
          • dumpConfiguration should be changed. It should be changed to the new JSON format.
          • DeprecatedQueueHierarchyBuilder should not hold on to an instance of the configuration

          Some minor comments on style:

          • Import only required classes and not all classes in a package.
          • getInnerQueues can be renamed as getDescendentQueues
          • DeprecatedQueueHierarchyBuilder.loadingDeprecatedResource can be loadQueues() or createQueues().
          • I think we are using "acls" as an XML attribute of the queues element to determine if ACLs are enabled or not. I would suggest we name it as aclsEnabled.
          • QueueManager.validate should be better documented to specify what it's validating.
          • There's an empty statement in QueueManager.populateProperties ( just a ';' )

          I am unable to decide on these points:

          • Do we need leafQueues and allQueues stored separately ?
          • Should we validate using an XSD ? I know you were planning on this. After looking at the code, I am thinking this makes good sense.
          • I am unable to decide if refreshConfiguration should be in the base class for QueueHierarchyBuilder, because it would need a Configuration in the deprecated case, and none in the new case.
          Show
          Hemanth Yamijala added a comment - I've started taking a look at this code, and have come till reviewing the framework changes. Here are some comments: I would suggest that we move out the parsing code for the new queues format into a separate class like QueueHierarchyBuilder, in order to keep the QueueManager class simple. I would also suggest that we make the deprecated and new class stick to a unified interface that provides APIs like: Queue getRoot(); boolean areAclsEnabled(); These can be used by the QueueManager on the appropriate builder depending on whether old configuration is available or not. This way, the initialization code (like in the constructor) of QueueManager will be consistent, irrespective of where the queue configuration is. Also, since we want to discard the mapred-site.xml loaded properties ASAP, we probably should parse and build the hierarchy from the constructor of the builder itself. createHierarchy should be refactored to parse leaf level queues and non leaf level queues separately. I would suggest something along the lines of: for each queue node: newQueue.name = parseName() newQueue.properties = parseProperties() if (node is a leaf level queue) { parseLeafQueueProperties() // here we parse ACLs, state and other properties. } else { for every queue node: recursively parse queue nodes. } Calling getInnerQueues without calling addChild can cause a null pointer because children is null. hashCode is overridden, but not equals I assume the order of child elements of an XML tag will be in the order in which they are defined in the file. I think we must explicitly ensure that elements come in a fixed order. For e.g. currently, we are not enforcing an order on how tags under the 'queue' node are defined. This could lead to cases where if a 'queue' element comes before the 'name' element, we would call parse hierarchy for the child queue without the parent's name. Shouldn't we check in populateProperties that the node is of type element. Since getNamedItem can be null, we should check for it in populateProperties. This will allow us to throw a better exception. In QueueManager.initialize, allQueues.putAll(leafQueues) seems unnecessary, since getInnerQueues already does this. setSchedulerInfo and getSchedulerInfo should work on all queues, not just leaf queues. The current refresh code is broken with reference to what we want to do long term. We should only change properties of leaf queues. Since we are planning to change the refresh model in MAPREDUCE-893 , I think for completeness sake the refresh of the code should build the entire hierarchy, but pickup only leaf level queues and copy properties like acls and state. Currently, since the refresh code is adding to the root queues, hierarchy could be affected which is not in the scope even of MAPREDUCE-893 . getJobQueueInfos is calling getJobQueueInfo twice. We should avoid this. Not a bug in the patch, as it was already there, but it will help to fix it. dumpConfiguration should be changed. It should be changed to the new JSON format. DeprecatedQueueHierarchyBuilder should not hold on to an instance of the configuration Some minor comments on style: Import only required classes and not all classes in a package. getInnerQueues can be renamed as getDescendentQueues DeprecatedQueueHierarchyBuilder.loadingDeprecatedResource can be loadQueues() or createQueues(). I think we are using "acls" as an XML attribute of the queues element to determine if ACLs are enabled or not. I would suggest we name it as aclsEnabled. QueueManager.validate should be better documented to specify what it's validating. There's an empty statement in QueueManager.populateProperties ( just a ';' ) I am unable to decide on these points: Do we need leafQueues and allQueues stored separately ? Should we validate using an XSD ? I know you were planning on this. After looking at the code, I am thinking this makes good sense. I am unable to decide if refreshConfiguration should be in the base class for QueueHierarchyBuilder, because it would need a Configuration in the deprecated case, and none in the new case.
          Hide
          Hemanth Yamijala added a comment -

          Opps. I didn't realize that my comments were posted partially frown, please ignore the first set. I am sorry as they are incomplete.

          Show
          Hemanth Yamijala added a comment - Opps. I didn't realize that my comments were posted partially frown , please ignore the first set. I am sorry as they are incomplete.
          Hide
          Hemanth Yamijala added a comment -

          I looked at most of the rest of the changes, including test cases. Here are some comments:

          • Test case testXMLParsing() can cover queue state also.
          • It should also verify that p1's child nodes are setup correctly.
          • We need a test case to test default values in mapred-queues.xml, which will just have the default queue, with no children and no properties, and acls disabled.
          • More error conditions to check:
            • queue without a name
            • properties without key or value attributes
            • empty file without any queues.
          • TestCapacitySchedulerConf tests must setup a map of queue names and properties and verify that the conversion of properties to the appropriate data types (like int, float, boolean etc). is happening correctly.

          A few minor nits:

          • CapacitySchedulerConf.addProperty can be called setProperties.
          • CapacitySchedulerConf.getProperty should check for the queue first before getting the queue properties. If we do this, we don't need to check for the queue not being null again, avoiding a query.
          • TestQueueManagerForHierarchialQueues.testXMLParsing needs correct formatting.
          Show
          Hemanth Yamijala added a comment - I looked at most of the rest of the changes, including test cases. Here are some comments: Test case testXMLParsing() can cover queue state also. It should also verify that p1's child nodes are setup correctly. We need a test case to test default values in mapred-queues.xml, which will just have the default queue, with no children and no properties, and acls disabled. More error conditions to check: queue without a name properties without key or value attributes empty file without any queues. TestCapacitySchedulerConf tests must setup a map of queue names and properties and verify that the conversion of properties to the appropriate data types (like int, float, boolean etc). is happening correctly. A few minor nits: CapacitySchedulerConf.addProperty can be called setProperties. CapacitySchedulerConf.getProperty should check for the queue first before getting the queue properties. If we do this, we don't need to check for the queue not being null again, avoiding a query. TestQueueManagerForHierarchialQueues.testXMLParsing needs correct formatting.
          Hide
          rahul k singh added a comment -

          Attaching the new patch with above comments taken care of , except few of them:

          createHierarchy should be refactored to parse leaf level queues and non leaf level queues separately. I would suggest something along the lines of:

          • The existing implementation is better , as it traverses much lesser time than the suggestion above.

          In QueueManager.initialize, allQueues.putAll(leafQueues) seems unnecessary, since getInnerQueues already does this.

          • getInnerQueues returns the list of inner queues . i.e . queues which have children. Hence the union of leafQueues and getInnerQueues would be the complete list. However in the deprecated conf case , leafQueue is sufficient.

          I assume the order of child elements of an XML tag will be in the order in which they are defined in the file. I think we must explicitly ensure that elements come in a fixed order. For e.g. currently, we are not enforcing an order on how tags under the 'queue' node are defined. This could lead to cases where if a 'queue' element comes before the 'name' element, we would call parse hierarchy for the child queue without the parent's name.

          • Done, but in different way. We still don't enforce the order ,but we make sure in code that parent is fully created before there is an attempt to create
            the child. Hence instead of depth first traversing we are now doing breadth first traversing.

          getInnerQueues can be renamed as getDescendentQueues

          • getInnerQueus method intent is to return list of queues which have children.

          Discussion points :

          Do we need leafQueues and allQueues stored separately ?

          • There are more than one occasions where we need only leafQueues to be used and more than one for only allQueues.
            Initial thought was to maintain the list of inner queues(i.e , queues which always have children) and leaf queues and generate there union
            whenever we need to access all the queues. Decided against it , as cost of putAll()(which we will call while union) would be costly if we need to do that everytime and also The cost of extra storage of leafQueues is not very high.

          Should we validate using an XSD ? I know you were planning on this. After looking at the code, I am thinking this makes good sense.

          • Yes i agree with this.

          I am unable to decide if refreshConfiguration should be in the base class for QueueHierarchyBuilder, because it would need a Configuration in the deprecated case, and none in the new case.

          • As of now we have single method refreshConfiguration(Configuration conf) and we pass null in case of HierarchyQueueBuilder class.

          We need a test case to test default values in mapred-queues.xml, which will just have the default queue, with no children and no properties, and acls disabled.

          dumpConfiguration is not yet done , will add 1 more patch to cover that.

          • Wiil do above 2 and upload the new patch.
          Show
          rahul k singh added a comment - Attaching the new patch with above comments taken care of , except few of them: createHierarchy should be refactored to parse leaf level queues and non leaf level queues separately. I would suggest something along the lines of: The existing implementation is better , as it traverses much lesser time than the suggestion above. In QueueManager.initialize, allQueues.putAll(leafQueues) seems unnecessary, since getInnerQueues already does this. getInnerQueues returns the list of inner queues . i.e . queues which have children. Hence the union of leafQueues and getInnerQueues would be the complete list. However in the deprecated conf case , leafQueue is sufficient. I assume the order of child elements of an XML tag will be in the order in which they are defined in the file. I think we must explicitly ensure that elements come in a fixed order. For e.g. currently, we are not enforcing an order on how tags under the 'queue' node are defined. This could lead to cases where if a 'queue' element comes before the 'name' element, we would call parse hierarchy for the child queue without the parent's name. Done, but in different way. We still don't enforce the order ,but we make sure in code that parent is fully created before there is an attempt to create the child. Hence instead of depth first traversing we are now doing breadth first traversing. getInnerQueues can be renamed as getDescendentQueues getInnerQueus method intent is to return list of queues which have children. Discussion points : Do we need leafQueues and allQueues stored separately ? There are more than one occasions where we need only leafQueues to be used and more than one for only allQueues. Initial thought was to maintain the list of inner queues(i.e , queues which always have children) and leaf queues and generate there union whenever we need to access all the queues. Decided against it , as cost of putAll()(which we will call while union) would be costly if we need to do that everytime and also The cost of extra storage of leafQueues is not very high. Should we validate using an XSD ? I know you were planning on this. After looking at the code, I am thinking this makes good sense. Yes i agree with this. I am unable to decide if refreshConfiguration should be in the base class for QueueHierarchyBuilder, because it would need a Configuration in the deprecated case, and none in the new case. As of now we have single method refreshConfiguration(Configuration conf) and we pass null in case of HierarchyQueueBuilder class. We need a test case to test default values in mapred-queues.xml, which will just have the default queue, with no children and no properties, and acls disabled. dumpConfiguration is not yet done , will add 1 more patch to cover that. Wiil do above 2 and upload the new patch.
          Hide
          Sreekanth Ramakrishnan added a comment -

          Uploading patch in-lieu of Rahul.

          Show
          Sreekanth Ramakrishnan added a comment - Uploading patch in-lieu of Rahul.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Had a quick look at the patch and have a comment. The refresh feature along with the new config file is committed to trunk(0.21) itself (HADOOP-5396). So, I think there is no need for handling deprecation of the old mapred-queue.xml properties. Deprecation is only needed for properties specified in old format in mapred-site.xml. With that, may be you should revise the class hierarchy of AbstractQueueBuilder. May be a single class would suffice. Thoughts?

          Show
          Vinod Kumar Vavilapalli added a comment - Had a quick look at the patch and have a comment. The refresh feature along with the new config file is committed to trunk(0.21) itself ( HADOOP-5396 ). So, I think there is no need for handling deprecation of the old mapred-queue.xml properties. Deprecation is only needed for properties specified in old format in mapred-site.xml. With that, may be you should revise the class hierarchy of AbstractQueueBuilder. May be a single class would suffice. Thoughts?
          Hide
          Vinod Kumar Vavilapalli added a comment -

          OK, my previous comment was invalid. Rahul corrected me offline that we are indeed handling deprecation for old format properties in mapred-site.xml. But as it was, there is a minor bug in DeprecatedQueueBuilder that threw me off the track. +146 line : conf.addResource(QueueManager.QUEUE_CONF_FILE_NAME); is not needed.

          Show
          Vinod Kumar Vavilapalli added a comment - OK, my previous comment was invalid. Rahul corrected me offline that we are indeed handling deprecation for old format properties in mapred-site.xml. But as it was, there is a minor bug in DeprecatedQueueBuilder that threw me off the track. +146 line : conf.addResource(QueueManager.QUEUE_CONF_FILE_NAME); is not needed.
          Hide
          Hemanth Yamijala added a comment -

          Started looking at the new patch. Some comments:

          • DeprecatedQueueBuilder should not load the queue resources from mapred-queues.xml unless it plans to parse the new format, which I think will complicate things. So, I suppose the right thing to do is to have DeprecatedQueueBuilder build a hierarchy from the old format configuration. If successful, we will simply discard the mapred-queues.xml file. This is a change from existing trunk behavior, but I think it is simpler and correct, thoughts ?
          • Also, DeprecatedQueueBuilder should build a hierarchy only if some configuration exists in mapred-site.xml. For instance, since it is looking up for configured queues with a default value of 'default', it will always end up loading a queue, which is not what we want. This can be checked possibly by returning a true / false in the checkDeprecation method.
          • We should simplify DeprecatedQueueBuilder to not initialize a root if there is no deprecated configuration. If we do that, then in QueueManager constructor, this check becomes redundant:
                    builder.getRoot().getChildren() == null 
            
          • Do we need the setAclsEnabled() call from QueueManager, as it is only setting the acls in the builder, which the builder object itself can do.
          • Document in the constructor for the QueueManager(String confFilePath) that this should stick to the format expected in mapred-queues.xml
          • In populateProperties, we are checking for KEY_TAG being null, but not value tag.
          • In getInnerQueues(), we are adding to the returned map, the list of children of the current queue, as in:
                    l.put(child.getName(),child);
            

            Doesn't this mean that we are going to return leaf level queues as well, and not just returns queues which have children ? For this same reason, it is behaving was similar to getDescendentQueues() in MAPREDUCE-824 which is why I suggested that name.

          • The refresh code could overwrite acls and state for some queues and if it gets a problem in parsing other queues, it would leave the system in an inconsistent state. So, q.setAcls and q.setState should be done only after refreshing the properties for all the queues.
          • HierarchyQueueBuilder.refreshQueues should not take a configuration object, as it is unused. (I suspect this may lead to a findbugs warning). That is the reason why I was confused about the signature of this method in the abstract class. The only solution I can think of is to handle refresh separately for the deprecated case. So, refreshQueues would not be part of the AbstractQueueBuilder interface. Instead, HierarchyQueueBuilder would have a refreshQueues that takes no parameter and DeprecatedQueueBuilder would have a refreshQueues with a Configuration parameter, and depending on the type of the builder used, the QueueManager would cast and make the right call. This is ugly, but noting that the refresh code is only at one place and the deprecated code would get removed in Hadoop 0.22, I think we can live with it for one release.
          • Atleast one class (Queue.java) is still Importing all classes.
          • Testcases in TestCapacitySchedulerConf may need a relook. Any test case that is reading properties that no longer are in capacity-scheduler.xml are suspect and can be knocked out.
          Show
          Hemanth Yamijala added a comment - Started looking at the new patch. Some comments: DeprecatedQueueBuilder should not load the queue resources from mapred-queues.xml unless it plans to parse the new format, which I think will complicate things. So, I suppose the right thing to do is to have DeprecatedQueueBuilder build a hierarchy from the old format configuration. If successful, we will simply discard the mapred-queues.xml file. This is a change from existing trunk behavior, but I think it is simpler and correct, thoughts ? Also, DeprecatedQueueBuilder should build a hierarchy only if some configuration exists in mapred-site.xml. For instance, since it is looking up for configured queues with a default value of 'default', it will always end up loading a queue, which is not what we want. This can be checked possibly by returning a true / false in the checkDeprecation method. We should simplify DeprecatedQueueBuilder to not initialize a root if there is no deprecated configuration. If we do that, then in QueueManager constructor, this check becomes redundant: builder.getRoot().getChildren() == null Do we need the setAclsEnabled() call from QueueManager, as it is only setting the acls in the builder, which the builder object itself can do. Document in the constructor for the QueueManager(String confFilePath) that this should stick to the format expected in mapred-queues.xml In populateProperties, we are checking for KEY_TAG being null, but not value tag. In getInnerQueues(), we are adding to the returned map, the list of children of the current queue, as in: l.put(child.getName(),child); Doesn't this mean that we are going to return leaf level queues as well, and not just returns queues which have children ? For this same reason, it is behaving was similar to getDescendentQueues() in MAPREDUCE-824 which is why I suggested that name. The refresh code could overwrite acls and state for some queues and if it gets a problem in parsing other queues, it would leave the system in an inconsistent state. So, q.setAcls and q.setState should be done only after refreshing the properties for all the queues. HierarchyQueueBuilder.refreshQueues should not take a configuration object, as it is unused. (I suspect this may lead to a findbugs warning). That is the reason why I was confused about the signature of this method in the abstract class. The only solution I can think of is to handle refresh separately for the deprecated case. So, refreshQueues would not be part of the AbstractQueueBuilder interface. Instead, HierarchyQueueBuilder would have a refreshQueues that takes no parameter and DeprecatedQueueBuilder would have a refreshQueues with a Configuration parameter, and depending on the type of the builder used, the QueueManager would cast and make the right call. This is ugly, but noting that the refresh code is only at one place and the deprecated code would get removed in Hadoop 0.22, I think we can live with it for one release. Atleast one class (Queue.java) is still Importing all classes. Testcases in TestCapacitySchedulerConf may need a relook. Any test case that is reading properties that no longer are in capacity-scheduler.xml are suspect and can be knocked out.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          I've started working on MAPREDUCE-893 with the latest patch on this JIRA. While doing that, I came out with some comments for this patch. I've tried to avoid any duplicate review comments, please ignore if you still find any. Most of them are minor and should not take too much time.

          AbstractQueueBuilder.java

          • Please add javadoc for this class.
          • I think it is no need for this class to be public.

          DeprecatedQueueBuilder.java

          • javadoc for the class
          • constructor: As Hemanth pointed out, QUEUE_CONF_FILE_NAME file should not be added to the conf object. We expect either old config or new config to be present. Not both.
          • Same with refreshQueues() (+146)
          • We don't need to parse ACLs if they are outright disabled. In your patch, it might happen because createQueues() parses ACLs in any case.
          • createQueues()
            • should be private
            • Throwable should not be caught like how it is done now. If we are not able to construct the QueueBuilder, I think it's OK for JT fails to start.
          • getRoot() and setRoot() seem to be redundant methods as they are already present in AbstractQueueBuilder with precisely the same code.
          • refreshQueues(): I am some how not comfortable with the big try{} catch(Throwable t) {} block here. If exceptions like NullPointerException are what it is guarding against, we should explicitly check for them and throw proper exceptions with appropriate messages ourselves. The enclosing code atleast is not yielding any declared exceptions.

          HierarchyQueueBuilder.java

          • javadoc for the class
          • No need for this class to be public
          • parseResource() should throw exception when QUEUES_TAG doesn't start the conf file. The patch just logs at fatal level now.
          • Rectify the character-case of the log/exception strings. Lower case is used throughout in some statements which looks bad.
          • createHierarchy():
            • Have a populateACLs() similar to populateProperties() ?
            • Should we support properties spanning across multiple <properties></properties> tags?
            • LOG level can be debug. Also, statement at +239 doesn't need to be repeated for each sub-queue.

          QueueManager.java

          • getRoot() and getRootQueues() - confusing names. Atleast to me.

          General

          • Port default acls and state to the new config file, atleast in commented XML.
          • Please add TestQueueManagerForHierarchialQueues and TestCapacityScheduler to the list of fast tests by appending them to the file src/test/commit-tests.
          • You can define a java string property name for mapred.queue.names and use it everywhere in your patch.
          • How about a new package Queues with all the queue related classes in the framework? Atleast the new ones?
          • There is a framework class HierarchyQueueBuilder and a CapacityScheduler class QueueHierarchyBuilder. To avoid any confusion, can the later just be QueueBuilder or something like that?
          Show
          Vinod Kumar Vavilapalli added a comment - I've started working on MAPREDUCE-893 with the latest patch on this JIRA. While doing that, I came out with some comments for this patch. I've tried to avoid any duplicate review comments, please ignore if you still find any. Most of them are minor and should not take too much time. AbstractQueueBuilder.java Please add javadoc for this class. I think it is no need for this class to be public. DeprecatedQueueBuilder.java javadoc for the class constructor: As Hemanth pointed out, QUEUE_CONF_FILE_NAME file should not be added to the conf object. We expect either old config or new config to be present. Not both. Same with refreshQueues() (+146) We don't need to parse ACLs if they are outright disabled. In your patch, it might happen because createQueues() parses ACLs in any case. createQueues() should be private Throwable should not be caught like how it is done now. If we are not able to construct the QueueBuilder, I think it's OK for JT fails to start. getRoot() and setRoot() seem to be redundant methods as they are already present in AbstractQueueBuilder with precisely the same code. refreshQueues(): I am some how not comfortable with the big try{} catch(Throwable t) {} block here. If exceptions like NullPointerException are what it is guarding against, we should explicitly check for them and throw proper exceptions with appropriate messages ourselves. The enclosing code atleast is not yielding any declared exceptions. HierarchyQueueBuilder.java javadoc for the class No need for this class to be public parseResource() should throw exception when QUEUES_TAG doesn't start the conf file. The patch just logs at fatal level now. Rectify the character-case of the log/exception strings. Lower case is used throughout in some statements which looks bad. createHierarchy(): Have a populateACLs() similar to populateProperties() ? Should we support properties spanning across multiple <properties></properties> tags? LOG level can be debug. Also, statement at +239 doesn't need to be repeated for each sub-queue. QueueManager.java getRoot() and getRootQueues() - confusing names. Atleast to me. General Port default acls and state to the new config file, atleast in commented XML. Please add TestQueueManagerForHierarchialQueues and TestCapacityScheduler to the list of fast tests by appending them to the file src/test/commit-tests. You can define a java string property name for mapred.queue.names and use it everywhere in your patch. How about a new package Queues with all the queue related classes in the framework? Atleast the new ones? There is a framework class HierarchyQueueBuilder and a CapacityScheduler class QueueHierarchyBuilder. To avoid any confusion, can the later just be QueueBuilder or something like that?
          Hide
          Vinod Kumar Vavilapalli added a comment -

          One of the earlier comments from Hemanth also seems to be missing in the latest patch.

          The current refresh code is broken with reference to what we want to do long term. We should only change properties of leaf queues. Since we are planning to change the refresh model in MAPREDUCE-893, I think for completeness sake the refresh of the code should build the entire hierarchy, but pickup only leaf level queues and copy properties like acls and state. Currently, since the refresh code is adding to the root queues, hierarchy could be affected which is not in the scope even of MAPREDUCE-893.

          Show
          Vinod Kumar Vavilapalli added a comment - One of the earlier comments from Hemanth also seems to be missing in the latest patch. The current refresh code is broken with reference to what we want to do long term. We should only change properties of leaf queues. Since we are planning to change the refresh model in MAPREDUCE-893 , I think for completeness sake the refresh of the code should build the entire hierarchy, but pickup only leaf level queues and copy properties like acls and state. Currently, since the refresh code is adding to the root queues, hierarchy could be affected which is not in the scope even of MAPREDUCE-893 .
          Hide
          Vinod Kumar Vavilapalli added a comment -

          With this patch, old capacity-scheduler specific queue configuration in capacity-scheduler.xml will no longer work. Will we provide backward compatibility or will this be an incompatible change w.r.t those config items?

          Show
          Vinod Kumar Vavilapalli added a comment - With this patch, old capacity-scheduler specific queue configuration in capacity-scheduler.xml will no longer work. Will we provide backward compatibility or will this be an incompatible change w.r.t those config items?
          Hide
          rahul k singh added a comment -

          Additional changes

          Changed JobSubmissionProtocol , introduced new public api , mentioned as part of MAPREDUCE - 860.
          Queue can only be accessed via its view.
          JobQueueInfo is changed to have package private api to access properties.

          Implemented all the comments except few:

          existing implementation of populating acls is better as we need to again pass the node.

          getRoot() and getRootQueues() - confusing names. Atleast to me.

          getRootQueues() is part of the interface exposed(860). So follows the same pattern.getRoot() is a utility
          method which is intended to be used in tests.

          How about a new package Queues with all the queue related classes in the framework? Atleast the new ones?

          might be good idea to do it , but iam not including it in this jira.

          There is a framework class HierarchyQueueBuilder and a CapacityScheduler class QueueHierarchyBuilder. To avoid any confusion, can the later just be QueueBuilder or something like that?

          think they are fine , as both class do generate the hierarchy.

          Show
          rahul k singh added a comment - Additional changes Changed JobSubmissionProtocol , introduced new public api , mentioned as part of MAPREDUCE - 860. Queue can only be accessed via its view. JobQueueInfo is changed to have package private api to access properties. Implemented all the comments except few: existing implementation of populating acls is better as we need to again pass the node. getRoot() and getRootQueues() - confusing names. Atleast to me. getRootQueues() is part of the interface exposed(860). So follows the same pattern.getRoot() is a utility method which is intended to be used in tests. How about a new package Queues with all the queue related classes in the framework? Atleast the new ones? might be good idea to do it , but iam not including it in this jira. There is a framework class HierarchyQueueBuilder and a CapacityScheduler class QueueHierarchyBuilder. To avoid any confusion, can the later just be QueueBuilder or something like that? think they are fine , as both class do generate the hierarchy.
          Hide
          Hemanth Yamijala added a comment -

          I am still to complete a bit of the review, but some preliminary comments:

          • Overall, I think javadocs can be improved. They seem a little too terse in most cases. Also, existing javadocs for QueueManager would need to change. Documentation in mapred-queues.xml can be made better too.
          • In DeprecatedHierarchyBuilder we are still not checking if ACLs are disabled before parsing them. Note though that this is being done for the QueueHierarchyBuilder.(Vinod's comment)
          • At the end, please review the log statements for verbosity level (most need to be removed / demoted to DEBUG), and for non-DEBUG messages, please review for presentability to the user (as noted by Vinod also)
          • I would recommend to add the format of acls and state to the default acls file.
          • TestCapacityScheduler should be added to fast tests.
          • Inline some of the one-liner methods. The objective is to reduce the number of APIs. New APIs will have the overhead of supporting and maintaining them once added.
          • I realized that dumpConfiguration in the old style configuration can be removed, because it is handled by the JobTracker.dumpConfiguration API. We can just file a follow-up JIRA for handling dumpConfiguration for the new style configuration - with an intent to push this patch in soon and unblock others.
          • I am rethinking about the QueueHierarchyBuilder hierarchy with an intent of simplification. Here's a proposal:
            class QueueHierarchyBuilder {
              Queue buildQueues() {
                // parse the new format.
                // return the root.
              }
            }
            
            class DeprecatedHierarchyBuilder extends QueueHierarchyBuilder {
              Queue root;
              DeprecatedHierarchyBuilder(Configuration conf) {
                 parseConfiguration(conf);
              }
              
              @Override
              Queue buildQueues() {
                return root;
              }
            
              void parseConfiguration(Configuration conf) {
                // parse the old format
                // fill the root.
              }
            }
            
            class QueueManager {
              static QueueHierarchyBuilder getInstance(Configuration conf) {
                if (conf.get("mapred.queue.names") != null) {
                  return new DeprecatedHierarchyBuilder(conf);
                } else {
                  return new QueueHierarchyBuilder();
                }
              }
            }
            

            Would this work ? Also, I think the builder instance can be discarded once it has got the root. I believe we can create a new instance for refreshing, because essentially that is still a parse operation.

          • I think the refresh algorithm can be simplified. Here's my suggestion:
            • Using a new builder instance, get a new root
            • Validate hierarchy by comparing the old tree and new tree of queues. I would suggest an API in Queue like hasSameHierarchyAs(Queue anotherQueue).
            • If things pass, switch the old root to the new root.
              Vis-a-vis the current implementation, I think it avoids a copy of the hierarchy - hence is more efficient. And it is simpler to understand IMHO.
          Show
          Hemanth Yamijala added a comment - I am still to complete a bit of the review, but some preliminary comments: Overall, I think javadocs can be improved. They seem a little too terse in most cases. Also, existing javadocs for QueueManager would need to change. Documentation in mapred-queues.xml can be made better too. In DeprecatedHierarchyBuilder we are still not checking if ACLs are disabled before parsing them. Note though that this is being done for the QueueHierarchyBuilder.(Vinod's comment) At the end, please review the log statements for verbosity level (most need to be removed / demoted to DEBUG), and for non-DEBUG messages, please review for presentability to the user (as noted by Vinod also) I would recommend to add the format of acls and state to the default acls file. TestCapacityScheduler should be added to fast tests. Inline some of the one-liner methods. The objective is to reduce the number of APIs. New APIs will have the overhead of supporting and maintaining them once added. I realized that dumpConfiguration in the old style configuration can be removed, because it is handled by the JobTracker.dumpConfiguration API. We can just file a follow-up JIRA for handling dumpConfiguration for the new style configuration - with an intent to push this patch in soon and unblock others. I am rethinking about the QueueHierarchyBuilder hierarchy with an intent of simplification. Here's a proposal: class QueueHierarchyBuilder { Queue buildQueues() { // parse the new format. // return the root. } } class DeprecatedHierarchyBuilder extends QueueHierarchyBuilder { Queue root; DeprecatedHierarchyBuilder(Configuration conf) { parseConfiguration(conf); } @Override Queue buildQueues() { return root; } void parseConfiguration(Configuration conf) { // parse the old format // fill the root. } } class QueueManager { static QueueHierarchyBuilder getInstance(Configuration conf) { if (conf.get( "mapred.queue.names" ) != null ) { return new DeprecatedHierarchyBuilder(conf); } else { return new QueueHierarchyBuilder(); } } } Would this work ? Also, I think the builder instance can be discarded once it has got the root. I believe we can create a new instance for refreshing, because essentially that is still a parse operation. I think the refresh algorithm can be simplified. Here's my suggestion: Using a new builder instance, get a new root Validate hierarchy by comparing the old tree and new tree of queues. I would suggest an API in Queue like hasSameHierarchyAs(Queue anotherQueue). If things pass, switch the old root to the new root. Vis-a-vis the current implementation, I think it avoids a copy of the hierarchy - hence is more efficient. And it is simpler to understand IMHO.
          Hide
          Hemanth Yamijala added a comment -

          One more comment I missed:
          We are not reloading configuration in the DeprecatedQueueBuilder case. Also, the test case should not use conf.set, but instead modify the existing file directly.

          Show
          Hemanth Yamijala added a comment - One more comment I missed: We are not reloading configuration in the DeprecatedQueueBuilder case. Also, the test case should not use conf.set, but instead modify the existing file directly.
          Hide
          rahul k singh added a comment -

          Incorporated all the comments except

          1.In DeprecatedHierarchyBuilder we are still not checking if ACLs are disabled before parsing them. Note though that this is being done for the QueueHierarchyBuilder.
          Lots of testcases esp. in TestQueueManager are written with an assumption that Map<String, AccessControlList> list is created for the Queue object all the time.
          esp in case of setting "mapred.acls.enabled" = true using conf.set . There are lots of NullPointerException if we dont generate this empty object. Hence not accommodating this comment , as it is a significant change in testcase and moreover for deprecated stuff and having this does empty Map<String,AccessControlList> doesn't effect the overall behaviour at all.

          Show
          rahul k singh added a comment - Incorporated all the comments except 1.In DeprecatedHierarchyBuilder we are still not checking if ACLs are disabled before parsing them. Note though that this is being done for the QueueHierarchyBuilder. Lots of testcases esp. in TestQueueManager are written with an assumption that Map<String, AccessControlList> list is created for the Queue object all the time. esp in case of setting "mapred.acls.enabled" = true using conf.set . There are lots of NullPointerException if we dont generate this empty object. Hence not accommodating this comment , as it is a significant change in testcase and moreover for deprecated stuff and having this does empty Map<String,AccessControlList> doesn't effect the overall behaviour at all.
          Hide
          rahul k singh added a comment -

          error in the above patch , attaching new one.

          Show
          rahul k singh added a comment - error in the above patch , attaching new one.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12418777/MAPREDUCE-861-5.patch
          against trunk revision 812002.

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

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

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 4 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 failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/42/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/42/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/42/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/42/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/12418777/MAPREDUCE-861-5.patch against trunk revision 812002. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 40 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 4 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 failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/42/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/42/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/42/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/42/console This message is automatically generated.
          Hide
          Hemanth Yamijala added a comment -

          This is getting really close, sans the issues with test-patch.

          • I still think QueueManager needs more java documentation - particularly explaining some aspects of hierarchical queues.
          • Also, the mapred-queues.xml can be better presented.
          • Not 100% sure about this, but will LOG.fatal cause an exception to be thrown ? If yes, can you check if that's the intended usage where you are using it ?
          • ACLs are stored inconsistently between Deprecated and hierarchical configuration. Specifically in the hierarchical configuration, they should be stored with the same key as in the hierarchical case, as the QueueManager treats them equally.
          Show
          Hemanth Yamijala added a comment - This is getting really close, sans the issues with test-patch. I still think QueueManager needs more java documentation - particularly explaining some aspects of hierarchical queues. Also, the mapred-queues.xml can be better presented. Not 100% sure about this, but will LOG.fatal cause an exception to be thrown ? If yes, can you check if that's the intended usage where you are using it ? ACLs are stored inconsistently between Deprecated and hierarchical configuration. Specifically in the hierarchical configuration, they should be stored with the same key as in the hierarchical case, as the QueueManager treats them equally.
          Hide
          Hemanth Yamijala added a comment -

          Oh, and I also meant to add that the refreshQueues api in queue manager can be simplified to work similarly in both the deprecated and the hierarchical case - thereby eliminating the need to check instance of the configuration parser. This will mean that the existing solution in deprecated queue configuration's case where we copy ACLs and state will be done away with and we'll just switch the roots. I think this is fair to do, given those are the only two properties we have. This will also fix an issue we currently have in the patch where the parsing is happening twice.

          Show
          Hemanth Yamijala added a comment - Oh, and I also meant to add that the refreshQueues api in queue manager can be simplified to work similarly in both the deprecated and the hierarchical case - thereby eliminating the need to check instance of the configuration parser. This will mean that the existing solution in deprecated queue configuration's case where we copy ACLs and state will be done away with and we'll just switch the roots. I think this is fair to do, given those are the only two properties we have. This will also fix an issue we currently have in the patch where the parsing is happening twice.
          Hide
          rahul k singh added a comment -

          attaching with new comments incorporated

          Show
          rahul k singh added a comment - attaching with new comments incorporated
          Hide
          Hadoop QA added a comment -

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

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/54/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/54/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/54/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/54/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/12419063/MAPREDUCE-861-6.patch against trunk revision 812546. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 43 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 failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/54/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/54/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/54/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/54/console This message is automatically generated.
          Hide
          rahul k singh added a comment -

          The testcases are failing due to non cleaning up of the build area while applying the patch . This is happening due to error in test-patch.sh script.
          refer jira HADOOP-6250

          running ant test locally.

          Show
          rahul k singh added a comment - The testcases are failing due to non cleaning up of the build area while applying the patch . This is happening due to error in test-patch.sh script. refer jira HADOOP-6250 running ant test locally.
          Hide
          rahul k singh added a comment -

          Ran the testcase with previous patch . MAPREDUCE-861-6.patch locally.
          2 testcases failed . TestRecoveryManager and TestJobQueueInformation.

          The new patch solves those problems ,
          ran ant test on this new patch , all passed.
          test-patch output is below:
          [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 46 new or modified tests.
          [exec]
          [exec] +1 javadoc. The javadoc tool did not generate any 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 release audit. The applied patch does not increase the total number of release audit warnings.
          [exec]
          [exec]
          [exec]
          [exec]
          [exec] ======================================================================
          [exec] ======================================================================
          [exec] Finished build.
          [exec] ======================================================================
          [exec] ======================================================================
          [exec]
          [exec]

          Show
          rahul k singh added a comment - Ran the testcase with previous patch . MAPREDUCE-861 -6.patch locally. 2 testcases failed . TestRecoveryManager and TestJobQueueInformation. The new patch solves those problems , ran ant test on this new patch , all passed. test-patch output is below: [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 46 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any 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 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec] [exec] [exec] [exec] ====================================================================== [exec] ====================================================================== [exec] Finished build. [exec] ====================================================================== [exec] ====================================================================== [exec] [exec]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12419265/MAPREDUCE-861-7.patch
          against trunk revision 813660.

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/28/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/28/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/28/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/28/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/12419265/MAPREDUCE-861-7.patch against trunk revision 813660. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 46 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 failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/28/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/28/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/28/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/28/console This message is automatically generated.
          Hide
          Hemanth Yamijala added a comment -

          I verified the changes in the last patch. They look fine to me. However, I think we do need to do something about the tests on hudson, because otherwise several test cases break badly. Can we fix HADOOP-6250 before we commit this ?

          Show
          Hemanth Yamijala added a comment - I verified the changes in the last patch. They look fine to me. However, I think we do need to do something about the tests on hudson, because otherwise several test cases break badly. Can we fix HADOOP-6250 before we commit this ?
          Hide
          Hemanth Yamijala added a comment -

          Running through Hudson again, now that HADOOP-6250 is committed, to check if all tests pass.

          Show
          Hemanth Yamijala added a comment - Running through Hudson again, now that HADOOP-6250 is committed, to check if all tests pass.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12419265/MAPREDUCE-861-7.patch
          against trunk revision 815249.

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

          +1 tests included. The patch appears to include 46 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/33/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/33/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/33/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/33/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/12419265/MAPREDUCE-861-7.patch against trunk revision 815249. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 46 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/33/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/33/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/33/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/33/console This message is automatically generated.
          Hide
          Hemanth Yamijala added a comment -

          I just committed this. Thanks, Rahul !

          Show
          Hemanth Yamijala added a comment - I just committed this. Thanks, Rahul !
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #41 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/41/)
          . Add support for hierarchical queues in the Map/Reduce framework. Contributed by Rahul Kumar Singh.

          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #41 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/41/ ) . Add support for hierarchical queues in the Map/Reduce framework. Contributed by Rahul Kumar Singh.
          Hide
          Hemanth Yamijala added a comment -

          Just a note that this commit could cause test cases in developer checked out workspaces to break because we changed to a new format of mapred-queues.xml. The fix is to simply remove mapred-queues.xml and let ant copy over the new file from the corresponding template.

          I did not mark this an incompatible change, because from a Hadoop release perspective, mapred-queues.xml was created only in Hadoop 0.21 with an older format.

          Show
          Hemanth Yamijala added a comment - Just a note that this commit could cause test cases in developer checked out workspaces to break because we changed to a new format of mapred-queues.xml. The fix is to simply remove mapred-queues.xml and let ant copy over the new file from the corresponding template. I did not mark this an incompatible change, because from a Hadoop release perspective, mapred-queues.xml was created only in Hadoop 0.21 with an older format.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #84 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/84/)
          . Add support for hierarchical queues in the Map/Reduce framework. Contributed by Rahul Kumar Singh.

          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #84 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/84/ ) . Add support for hierarchical queues in the Map/Reduce framework. Contributed by Rahul Kumar Singh.

            People

            • Assignee:
              rahul k singh
              Reporter:
              Hemanth Yamijala
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development