Eric Payne updated YARN-2932:
    Attachment: YARN-2932.v2.txt

Thanks very much, [~leftnoteasy], for your thorough review of this patch and 
for your helpful comments.

1) Since the QUEUE_PREEMPTION_DISABLED is an option for CS, I suggest to make 
it as a member of CapacitySchedulerConfiguration, like 
getUserLimitFactor/setUserLimit, etc. This will void some String operations.
This is a good idea. I added {{isQueuePreemptable}} and 
{{setQueuePreemptable}}. For {{isQueuePreemptable}}, I needed to add a default 
value parameter because the default for the queue at a particular level should 
be whatever its parent's value is.

2) Rename {{context}} in {{AbstractCSQueue}} to name like {{csContext}} since 
we have {{rmContext}}

3) I suggest to add a member var like {{preemptable}} to {{AbstractCSQueue}}, 
instead of calling:
+  @Private
+  public boolean isPreemptable() {
+    return context.getConfiguration().isPreemptable(getQueuePath());
+  }
The implementation of {{CSConfiguration.isPreemptable(..)}} seems too complex 
to me. {{CSConfiguration}} should only care about value of configuration file, 
such logic should put to {{AbstractCSQueue.setupQueueConfigs(...)}}
I moved the logic to {{AbstractCSQueue.setupQueueConfigs(...)}}, and you are 
right. It is much cleaner that way. Thanks!

4) It's better to web UI name (preemptable) and configuration name 
(disable_preemption) consistent. I prefer "preemptable" personally.
Yes, it is less confusing that way. In this patch, the only things that worry 
about the {{disable_preemption}} property are the internals of the 
{{CSConfiguration}} methods. The APIs are now all asking whether or not the 
queue is preemptable.

5) {{testIsPreemptable}} should be a part of {{TestCapacityScheduler}} instead 
of putting it to {{TestProportionalCapacityPreemptionPolicy}}.
Thanks. I moved the test to {{testIsPreemptable}}. However, since the interface 
for changing a queue's preemptability changed, there were also several changes 
to {{TestProportionalCapacityPreemptionPolicy}}.

6) In {{ProportionalCapacityPreemptionPolicy.cloneQueues}}, preemptable field 
should get from Queue instead of getting from configuration.

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