[JENKINS-21316] PrioritySorter wrongly assumes Queue.Task is Job

* Updated fix provided for [JENKINS-21310] to exit early * Updated javadoc to clearify responsibilities
jenkinsci · Jan 10, 2014 · 5b6e7c6 · 5b6e7c6
1 parent 9c3fc00
commit 5b6e7c6
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 29 deletions.
diff --git a/src/main/java/jenkins/advancedqueue/PriorityConfiguration.java b/src/main/java/jenkins/advancedqueue/PriorityConfiguration.java
@@ -71,8 +71,7 @@
  * @since 2.0
  */
 @Extension
-public class PriorityConfiguration extends Descriptor<PriorityConfiguration> implements RootAction,
-		Describable<PriorityConfiguration> {
+public class PriorityConfiguration extends Descriptor<PriorityConfiguration> implements RootAction, Describable<PriorityConfiguration> {
 
 	private final static Logger LOGGER = Logger.getLogger(PriorityConfiguration.class.getName());
 
@@ -119,13 +118,13 @@ public String getUrlName() {
 		}
 		return "advanced-build-queue";
 	}
-	
+
 	private boolean checkActive() {
 		PrioritySorterConfiguration configuration = PrioritySorterConfiguration.get();
-		if(configuration.getLegacyMode()) {
+		if (configuration.getLegacyMode()) {
 			return false;
 		}
-		if(configuration.getOnlyAdminsMayEditPriorityConfiguration()) {
+		if (configuration.getOnlyAdminsMayEditPriorityConfiguration()) {
 			return Jenkins.getInstance().getACL().hasPermission(Jenkins.ADMINISTER);
 		}
 		return true;
@@ -192,7 +191,7 @@ public FormValidation doCheckJobPattern(@QueryParameter String value) throws IOE
 		}
 		return FormValidation.ok();
 	}
-	
+
 	public PriorityConfigurationCallback getPriority(Queue.Item item, PriorityConfigurationCallback priorityCallback) {
 		SecurityContext saveCtx = ACL.impersonate(ACL.SYSTEM);
 		try {
@@ -201,32 +200,38 @@ public PriorityConfigurationCallback getPriority(Queue.Item item, PriorityConfig
 			SecurityContextHolder.setContext(saveCtx);
 		}
 	}
-
-	private PriorityConfigurationCallback getPriorityInternal(Queue.Item item,
-			PriorityConfigurationCallback priorityCallback) {
-		Queue.Task job = item.task;
+
+	private PriorityConfigurationCallback getPriorityInternal(Queue.Item item, PriorityConfigurationCallback priorityCallback) {
+
+		if (!(item.task instanceof Job)) {
+			// Not a job generally this mean that this is a lightweight task so
+			// priority doesn't really matter - returning default priority
+			priorityCallback.addDecisionLog(0, "Queue.Item is not a Job - Assigning global default priority");
+			return priorityCallback.setPrioritySelection(PrioritySorterConfiguration.get().getStrategy().getDefaultPriority());
+		}
+
+		Job<?, ?> job = (Job<?, ?>) item.task;
 
 		// [JENKINS-8597]
-		// For MatrixConfiguration use the latest assigned Priority from the MatrixProject
+		// For MatrixConfiguration use the latest assigned Priority from the
+		// MatrixProject
 		if (job instanceof MatrixConfiguration) {
 			MatrixProject matrixProject = ((MatrixConfiguration) job).getParent();
 			priorityCallback.addDecisionLog(0, "Job is MatrixConfiguration [" + matrixProject.getName() + "] ...");
 			ItemInfo itemInfo = QueueItemCache.get().getItem(matrixProject.getName());
-			// Can be null (for example) at startup when the MatrixBuild got lost (was running at
+			// Can be null (for example) at startup when the MatrixBuild got
+			// lost (was running at
 			// restart)
 			if (itemInfo != null) {
-				priorityCallback.addDecisionLog(0, "MatrixProject found in cache, using priority from queue-item ["
-						+ itemInfo.getItemId() + "]");
-				return priorityCallback.setPrioritySelection(itemInfo.getPriority(), itemInfo.getJobGroupId(),
-						itemInfo.getPriorityStrategy());
+				priorityCallback.addDecisionLog(0, "MatrixProject found in cache, using priority from queue-item [" + itemInfo.getItemId() + "]");
+				return priorityCallback.setPrioritySelection(itemInfo.getPriority(), itemInfo.getJobGroupId(), itemInfo.getPriorityStrategy());
 			}
 			priorityCallback.addDecisionLog(0, "MatrixProject not found in cache, assigning global default priority");
-			return priorityCallback.setPrioritySelection(PrioritySorterConfiguration.get().getStrategy()
-					.getDefaultPriority());
+			return priorityCallback.setPrioritySelection(PrioritySorterConfiguration.get().getStrategy().getDefaultPriority());
 		}
 
-		if (job instanceof Job && PrioritySorterConfiguration.get().getAllowPriorityOnJobs()) {
-			AdvancedQueueSorterJobProperty priorityProperty = ((Job<?,?>) job).getProperty(AdvancedQueueSorterJobProperty.class);
+		if (PrioritySorterConfiguration.get().getAllowPriorityOnJobs()) {
+			AdvancedQueueSorterJobProperty priorityProperty = ((Job<?, ?>) job).getProperty(AdvancedQueueSorterJobProperty.class);
 			if (priorityProperty != null && priorityProperty.getUseJobPriority()) {
 				int priority = priorityProperty.priority;
 				if (priority == PriorityCalculationsUtil.getUseDefaultPriorityPriority()) {
@@ -243,11 +248,11 @@ private PriorityConfigurationCallback getPriorityInternal(Queue.Item item,
 		}
 		//
 		priorityCallback.addDecisionLog(0, "Assigning global default priority");
-		return priorityCallback.setPrioritySelection(PrioritySorterConfiguration.get().getStrategy()
-				.getDefaultPriority());
+		return priorityCallback.setPrioritySelection(PrioritySorterConfiguration.get().getStrategy().getDefaultPriority());
 	}
 
-    // TODO a simple jobName will not work for jobs in folders, and would be meaningless for non-Job Queue.Task’s
+	// TODO a simple jobName will not work for jobs in folders, and would be
+	// meaningless for non-Job Queue.Task’s
 	public JobGroup getJobGroup(PriorityConfigurationCallback priorityCallback, String jobName) {
 		for (JobGroup jobGroup : jobGroups) {
 			priorityCallback.addDecisionLog(0, "Evaluating JobGroup [" + jobGroup.getId() + "] ...");
@@ -291,22 +296,19 @@ public JobGroup getJobGroup(PriorityConfigurationCallback priorityCallback, Stri
 		return null;
 	}
 
-	private PriorityConfigurationCallback getPriorityForJobGroup(PriorityConfigurationCallback priorityCallback,
-			JobGroup jobGroup, Queue.Item item) {
+	private PriorityConfigurationCallback getPriorityForJobGroup(PriorityConfigurationCallback priorityCallback, JobGroup jobGroup, Queue.Item item) {
 		int priority = jobGroup.getPriority();
 		PriorityStrategy reason = null;
 		if (jobGroup.isUsePriorityStrategies()) {
 			priorityCallback.addDecisionLog(2, "Evaluating strategies ...");
 			List<JobGroup.PriorityStrategyHolder> priorityStrategies = jobGroup.getPriorityStrategies();
 			for (JobGroup.PriorityStrategyHolder priorityStrategy : priorityStrategies) {
 				PriorityStrategy strategy = priorityStrategy.getPriorityStrategy();
-				priorityCallback.addDecisionLog(3, "Evaluating strategy ["
-						+ strategy.getDescriptor().getDisplayName() + "] ...");
+				priorityCallback.addDecisionLog(3, "Evaluating strategy [" + strategy.getDescriptor().getDisplayName() + "] ...");
 				if (strategy.isApplicable(item)) {
 					priorityCallback.addDecisionLog(4, "Strategy is applicable");
 					int foundPriority = strategy.getPriority(item);
-					if (foundPriority > 0
-							&& foundPriority <= PrioritySorterConfiguration.get().getStrategy().getNumberOfPriorities()) {
+					if (foundPriority > 0 && foundPriority <= PrioritySorterConfiguration.get().getStrategy().getNumberOfPriorities()) {
 						priority = foundPriority;
 						reason = strategy;
 						break;

diff --git a/src/main/java/jenkins/advancedqueue/priority/PriorityStrategy.java b/src/main/java/jenkins/advancedqueue/priority/PriorityStrategy.java
@@ -27,7 +27,9 @@
 import hudson.ExtensionPoint;
 import hudson.model.Describable;
 import hudson.model.Descriptor;
+import hudson.model.Job;
 import hudson.model.Queue;
+import hudson.model.Queue.Item;
 import jenkins.model.Jenkins;
 
 /**
@@ -36,8 +38,25 @@
  */
 public abstract class PriorityStrategy implements ExtensionPoint, Describable<PriorityStrategy> {
 
+	/** 
+	 * Method that checks if strategy can assign a priority to the provided {@link Item}
+	 * 
+	 * The caller garanties that the {@link Item#task} is a {@link Job}
+	 * 
+	 * @param item the {@link Item} to check
+	 * @return <code>true</code> if the {@link PriorityStrategy} is applicable else <code>false</code>
+	 */
 	abstract public boolean isApplicable(Queue.Item item);
 
+	/** 
+	 * Method that that return the priority that should be used for this {@link Item}, this method is only called id
+	 * {@link PriorityStrategy#isApplicable(Item)} returned true
+	 * 
+	 * The caller garanties that the {@link Item#task} is a {@link Job}
+	 * 
+	 * @param item the {@link Item} to check
+	 * @return the priority to be used by the provided {@link Item}
+	 */
 	abstract public int getPriority(Queue.Item item);
 
 	abstract public void numberPrioritiesUpdates(int oldNumberOfPriorities, int newNumberOfPriorities);