[
https://issues.apache.org/jira/browse/YARN-10506?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17266081#comment-17266081
]
Gergely Pollak commented on YARN-10506:
---------------------------------------
Thank you [~gandras] for the fix, here are a few comments, most of them can
wait for a followup JIRA:
*CapacitySchedulerAutoQueueHandler class*:
Why is it a separate class, this is the functionality of the actual queue, I
think it would have been better to extend the parentQueue and add this
functionality there, this is a class which automagicly changes the ParentQueue
class and enhances it with new functionality, but I think it would be better to
solve this via inheritance. Also that would make differentiating between
ParentQueue and the DynamicParentQueue much easier. Probably we should fix it
in a followup JIRA.
*CapacitySchedulerAutoQueueHandler#getQueue*
This method is unnecessary, since CSQueueStore.get method handles null paths,
and returns null. See CapacitySchedulerQueueManager#getQueue and
CSQueueStore#get
*CapacitySchedulerAutoQueueHandler#autoCreateQueue*
I might be wrong, but this is concerning:
{code:java}
CSQueue existingQueueCandidate =
getQueue(queueCandidateContext.getQueue());{code}
The getQueue will always return the leaf name of the queue, which might be
ambiguous so this might return null even if the queue exists on the path we are
checking, and might identify an already existing queue invalid.
As a rule of thumb, wherever possible use the full path of the queues please!
If we have the following structure:
{code:java}
root
+--alice
| +--test
| +--dev
+--bob
| +--test
| +--dev
{code}
Where both _dev_ and _test_ queues are dynamic parents, then when I try to
submit something to root.bob.dev.something, this part of the code
{code:java}
ApplicationPlacementContext queueCandidateContext = parentContext; //
(Parent context is root.bob.dev)
CSQueue existingQueueCandidate =
getQueue(queueCandidateContext.getQueue()); //queueCandidateContext.getQueue()
= 'dev'
getQueue(queueCandidateContext.getQueue()) == null //because dev is
ambiguous
{code}
Will identify root.bob.dev as a not existing queue.
So please use _ApplciationPlacementContext#getFullQueuePath_
*CapacitySchedulerAutoQueueHandler#isDynamicQueue*
Is not fool/future proof, while I'm aware currently only the AbstractCSQueue
class implements the interface, but still it would be better to check before
casting it to AbstractCSQueue. Or move the isDynamic down to the interface,
then you don't have to cast
> Update queue creation logic to use weight mode and allow the flexible
> static/dynamic creation
> ---------------------------------------------------------------------------------------------
>
> Key: YARN-10506
> URL: https://issues.apache.org/jira/browse/YARN-10506
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Benjamin Teke
> Assignee: Andras Gyori
> Priority: Major
> Attachments: YARN-10506-006-10504-010.patch,
> YARN-10506-007-10504-010.patch, YARN-10506-008.patch, YARN-10506-010.patch,
> YARN-10506-012.patch, YARN-10506-013.patch, YARN-10506.001.patch,
> YARN-10506.002.patch, YARN-10506.003.patch, YARN-10506.004.patch,
> YARN-10506.005.patch, YARN-10506.006-combined.patch, YARN-10506.006.patch,
> YARN-10506.007.patch, YARN-10506.009.patch, YARN-10506.011.patch,
> YARN-10506.014.patch
>
>
> The queue creation logic should be updated to use weight mode and support the
> flexible creation.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
