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