Eric Payne commented on YARN-2932:

[~leftnoteasy], thanks very much for your review and comments:

bq. 1. Rename {{isQueuePreemptable}} to {{getQueuePreemptable}} for 
getter/setter consistency in {{CapacitySchedulerConfiguration}}

bq. 2. Should consider queue reinitialize when queue preemptable in 
configuration changes (See {{TestQueueParsing}}). And it's best to add a test 
for verify that.
I'm sorry. I don't understand what you mean by the use of the word "consider." 
Calling {{CapacityScheduler.reinitialize}} will follow the queue hierarchy down 
and eventually call {{AbstractCSQueue#setupQueueConfigs}} for every queue, so I 
don't think there is any additional code needed, unless I'm missing something. 
Were you just saying that I need to add a test case for that?

3. It's better to remove the {{defaultVal}} parameter in 
public boolean isQueuePreemptable(String queue, boolean defaultVal) 
And the default_value should be placed in {{CapacitySchedulerConfiguration}}, 
like other queue configuration options.
I understand what you trying to do is moving some logic from queue to 
{{CapacitySchedulerConfiguration}}, but I still think it's better to keep the 
{{CapacitySchedulerConfiguration}} simply gets some values from configuration 
The problem is that without the {{defaultval}} parameter, 
{{AbstractCSQueue#isQueuePathHierarchyPreemptable}} can't tell if the queue has 
explicitly set its preemptability or if it is just returning the default. For 
root: disable_preemption = true
root.A: disable_preemption (the property is not set)
root.B: disable_preemption = false (the property is explicitly set to false)
Let's say the {{getQueuePreemptable}} interface is changed to remove the 
{{defaultVal}} parameter, and that when {{getQueuePreemptable}} calls 
{{getBoolean}}, it uses {{false}} as the default.

# {{getQueuePreemptable}} calls {{getBoolean}} on {{root}}
## {{getBoolean}} returns {{true}} because the {{disable_preemption}} property 
is set to {{true}}
## {{getQueuePreemptable}} inverts {{true}} and returns {{false}} (That is, 
{{root}} has preemption disabled, so it is not preemptable).
# {{getQueuePreemptable}} calls {{getBoolean}} on {{root.A}}
## {{getBoolean}} returns {{false}} because there is no {{disable_preemption}} 
property set for this queue, so {{getBoolean}} returns the default.
## {{getQueuePreemptable}} inverts {{false}} and returns {{true}}
# {{getQueuePreemptable}} calls {{getBoolean}} on {{root.B}}
## {{getBoolean}} returns {{false}} because {{disable_preemption}} property is 
set to {{false}} for this queue
## {{getQueuePreemptable}} inverts {{false}} and returns {{true}}

At this point, {{isQueuePathHierarchyPreemptable}} needs to know if it should 
use the default preemption from {{root}} or if it should use the value from 
each child queue. In the case of {{root.A}}, the value from {{root}} 
({{false}}) should be used because {{root.A}} does not have the property set. 
In the case of {{root.B}}, the value should be the one returned for {{root.B}} 
({{true}}) because it is explicitly set. But, since both {{root.A}} and 
{{root.B}} both returned {{true}}, {{isQueuePathHierarchyPreemptable}} can't 
tell the difference. Does that make sense?

> Add entry for preemption setting to queue status screen and startup/refresh 
> logging
> -----------------------------------------------------------------------------------
>                 Key: YARN-2932
>                 URL: https://issues.apache.org/jira/browse/YARN-2932
>             Project: Hadoop YARN
>          Issue Type: Bug
>    Affects Versions: 3.0.0, 2.7.0
>            Reporter: Eric Payne
>            Assignee: Eric Payne
>         Attachments: YARN-2932.v1.txt, YARN-2932.v2.txt, YARN-2932.v3.txt
> YARN-2056 enables the ability to turn preemption on or off on a per-queue 
> level. This JIRA will provide the preemption status for each queue in the 
> {{HOST:8088/cluster/scheduler}} UI and in the RM log during startup/queue 
> refresh.

This message was sent by Atlassian JIRA

Reply via email to