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