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-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
