hadoop-mapreduce-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vinod K V (JIRA)" <j...@apache.org>
Subject [jira] Commented: (MAPREDUCE-861) Modify queue configuration format and parsing to support a hierarchy of queues.
Date Mon, 31 Aug 2009 09:22:32 GMT

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

Vinod K V commented on MAPREDUCE-861:
-------------------------------------

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?

> 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.


Mime
View raw message