Jason Lowe commented on YARN-2004:

I don't think it matters if we allow negative priorities.  Numbers are easy to 
compare, even when negative numbers enter the mix.  If people feel strongly 
that negative numbers are too confusing to compare then we can force them to be 
non-negative or even non-zero if that is also too confusing.  If we allow 
negative priorities then I suggest we use zero as the default priority.  Any 
positive priority will be higher and scheduled before the default priority.  
Any negative priority will be lower and scheduled after the default priority.  
Simple.  If we don't allow negative priorities then be sure to set the default 
such that users can set applications to be lower priority than the default.

As for per-user priority settings, again I'm advocating for simplicity first.  
We don't need per-user settings to get the basic, and highly-requested feature 
working first.  Adding this feature later should not disrupt any of the initial 
APIs, as these would be separate, admin APIs (or just configs) that would not 
affect app submission.  If per-user priority defaults are needed after the 
basic priority functionality is there then we can add it then.

I'm not sure about the current state of the patch and how it relates to 
YARN-2003.  I see there are default priorities, but I don't see them being 
really used in either this patch nor in the latest YARN-2003 patch.  I'm also 
wondering if we really need per-queue priority limits.  Currently application 
priorities have no effect _between_ queues, therefore I don't understand why we 
would want/need to limit application priorities in one queue vs. another queue. 
 Maybe I'm missing the use-case for this feature.  If we don't have a solid 
use-case for it then we should not add it until we need it.  Again, this is 
something we can always add later.

There appear to be some missing NULL checks when it comes to priorities in the 
following code:
+      if (a1.getApplicationPriority() != null
+          && !a1.getApplicationPriority().equals(a2.getApplicationPriority())) 
+        return a1.getApplicationPriority().compareTo(
+            a2.getApplicationPriority());
+      }
If a1.getApplicationPriority() returns non-null but a2.getApplicationPriority() 
returns null then I think we will NPE, as Priority.compareTo has no null checks.

Nit: I'd like to see the submitted application priority logged along with other 
essential app details when the app is submitted rather than a separate log 
message just for priority.  The RM log is already too wordy, and this INFO 
message will add to it.  Maybe it should just be a debug log?
+    LOG.info("Submitted priority '" + priority.getPriority()
+        + "' is acceptable in queue :" + queueName + "for application:"
+        + applicationId);

> Priority scheduling support in Capacity scheduler
> -------------------------------------------------
>                 Key: YARN-2004
>                 URL: https://issues.apache.org/jira/browse/YARN-2004
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: capacityscheduler
>            Reporter: Sunil G
>            Assignee: Sunil G
>         Attachments: 0001-YARN-2004.patch, 0002-YARN-2004.patch, 
> 0003-YARN-2004.patch, 0004-YARN-2004.patch, 0005-YARN-2004.patch
> Based on the priority of the application, Capacity Scheduler should be able 
> to give preference to application while doing scheduling.
> Comparator<FiCaSchedulerApp> applicationComparator can be changed as below.   
> 1.    Check for Application priority. If priority is available, then return 
> the highest priority job.
> 2.    Otherwise continue with existing logic such as App ID comparison and 
> then TimeStamp comparison.

This message was sent by Atlassian JIRA

Reply via email to