[GitHub] [camel-k] squakez commented on a diff in pull request #4523: fix(#592): Introduce build order strategy

["squakez (via GitHub)" <gi...@apache.org>]

squakez commented on code in PR #4523:
URL: https://github.com/apache/camel-k/pull/4523#discussion_r1252064352


##########
pkg/controller/build/build_monitor.go:
##########
@@ -84,18 +85,26 @@ func (bm *Monitor) canSchedule(ctx context.Context, c ctrl.Reader, build *v1.Bui
 		return false, err
 	}
 
-	// Emulate a serialized working queue to only allow one build to run at a given time.
-	// This is currently necessary for the incremental build to work as expected.
-	// We may want to explicitly manage build priority as opposed to relying on
-	// the reconciliation loop to handle the queuing.
-	for _, b := range builds.Items {
-		if b.Status.Phase == v1.BuildPhasePending || b.Status.Phase == v1.BuildPhaseRunning {
-			// Let's requeue the build in case one is already running
-			return false, nil
-		}
+	var allowed bool
+	switch bm.buildOrderStrategy {
+	case v1.BuildOrderStrategyFIFO:
+		// Check on builds that have been created before the current build and grant precedence if any.
+		allowed = !builds.HasScheduledBuildsBefore(build)
+	case v1.BuildOrderStrategyDependencies:
+		// Check on the Integration dependencies and see if we should queue the build in order to leverage incremental builds
+		// because there is already another build in the making that matches the requirements
+		allowed = !builds.HasMatchingBuild(build)
+	case v1.BuildOrderStrategySequential:
+		// Emulate a serialized working queue to only allow one build to run at a given time.
+		// Let's requeue the build in case one is already running
+		allowed = !builds.HasRunningBuilds()
+	default:
+		Log.WithValues("request-namespace", requestNamespace, "request-name", requestName, "order-strategy", bm.buildOrderStrategy).
+			ForBuild(build).Infof("Unsupported build order strategy (%s) - the build gets enqueued", bm.buildOrderStrategy)
+		allowed = false
 	}
 
-	return true, nil
+	return allowed, nil

Review Comment:
   It would be nice if we could return the reason why a build is allowed or not, so that we include it in the condition. In particular, we may show the user that a build is queued waiting for a depending build to complete. However I'm not sure how much complicated is to bubble up this information, you please evaluate if it makes sense to include it or defer to a later development. I think it also would simplify the e2e test (ie, we just need to check that a build is queued up with the proper condition without waiting it to complete)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org