Mailing-List: contact mapreduce-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: mapreduce-issues@hadoop.apache.org
Message-ID: <1534396240.1251704612799.JavaMail.jira@brutus>
Date: Mon, 31 Aug 2009 00:43:32 -0700 (PDT)
From: "Hemanth Yamijala (JIRA)" <jira@apache.org>
To: mapreduce-issues@hadoop.apache.org
Subject: [jira] Commented: (MAPREDUCE-861) Modify queue configuration format
 and parsing to support a hierarchy of queues.
In-Reply-To: <1467360010.1250163735299.JavaMail.jira@brutus>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ https://issues.apache.org/jira/browse/MAPREDUCE-861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12749406#action_12749406 ] 

Hemanth Yamijala commented on MAPREDUCE-861:
--------------------------------------------

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:
{code}
        builder.getRoot().getChildren() == null 
{code}
- 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:
{code}
        l.put(child.getName(),child);
{code}
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.


> Modify queue configuration format and parsing to support a hierarchy of queues.
> -------------------------------------------------------------------------------
>
>                 Key: MAPREDUCE-861
>                 URL: https://issues.apache.org/jira/browse/MAPREDUCE-861
>             Project: Hadoop Map/Reduce
>          Issue Type: Sub-task
>            Reporter: Hemanth Yamijala
>            Assignee: rahul k singh
>         Attachments: MAPREDUCE-861-1.patch, MAPREDUCE-861-2.patch
>
>
> 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.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.