[
https://issues.apache.org/jira/browse/MAPREDUCE-861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12751318#action_12751318
]
Hemanth Yamijala commented on MAPREDUCE-861:
--------------------------------------------
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:
{code}
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();
}
}
}
{code}
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.
> 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-861-3.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.
