Start untangling orchestrator#1739
Draft
mnonnenmacher wants to merge 10 commits intomainfrom
Draft
Conversation
Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>
Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>
Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>
This makes the code more explicit and helps with upcoming refactorings. Also, the function name was confusing because it was not only responsible for scheduling jobs but also for executing the `onFailure` handler from the caller. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>
The word "current" does not carry any meaning in the context of the function. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>
Use the `WorkerScheduleInfo` enum instead of the `Endpoint` class to define the dependencies between jobs. This slightly simplifies the code and improves type safety. It also makes the companion object of `WorkerScheduleInfo` obsolete. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>
Extract the logic to determine which jobs should run next into a new `OrtRunInfo` class to be able to test it independently. The class will be taken into use in a follow-up commit. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>
If the analyzer did not run, it makes no sense to run the reporter worker as there is no data to report. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>
Add a new function `scheduleNextJobs` that uses the previously introduced `OrtRunInfo` to determine which jobs need to be scheduled and delete all now unused functions from the previous implementation. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>
With the introduction of the `JobStatus.final` property in 4d23673 the `WorkerJob.isCompleted()` helper function is not required anymore. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.com>
oheger-bosch
reviewed
Jan 8, 2025
| import org.eclipse.apoapsis.ortserver.model.JobStatus | ||
|
|
||
| /** A class to store the required information to determine which jobs can be run. */ | ||
| internal class OrtRunInfo( |
Contributor
There was a problem hiding this comment.
While I like the idea to extract the scheduling logic to a dedicated class, I have some problems with the current implementation:
- IIUC, this class now contains the scheduling logic and is responsible to determine the next jobs that should run. This should also be reflected by the class name.
OrtRunInfois meaningless in this context and rather reminds of a data model class. - The relation between this class and
WorkerScheduleContextis unclear.Orchestratornow creates aWorkerScheduleContext, and with the help of this context, anOrtRunInfo. This is because the latter has its own state derived from the context (this is not really untangling). It would be better ifOrtRunInfowas stateless and only implemented the scheduling strategy. ThegetNextJobs()function could be passed aWorkerScheduleContextinfo object and obtain all required information from there.
| }, | ||
|
|
||
| REPORTER(ReporterEndpoint, runsAfter = listOf(EVALUATOR), runAfterFailure = true) { | ||
| REPORTER(ReporterEndpoint, dependsOn = listOf(ANALYZER), runsAfter = listOf(EVALUATOR), runAfterFailure = true) { |
Contributor
There was a problem hiding this comment.
It was a deliberate decision that the Reporter step should always be executed to enable use cases like an "ORT run report" containing information about the whole run with its successful and failing steps. So, I would be reluctant to say that it generally makes no sense to run the reporter after a failed analyzer step. In every case, the type "fix" is not correct for this commit because this is no bug, but the behavior was by design.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The few classes in the
orchestratorcurrently have a very high degree of coupling and no clear separation of concerns. For example:WorkerScheduleInfoand partly inWorkerScheduleContext.Orchestrator,WorkerScheduleContext, andWorkerScheduleInfo.Orchestrator,WorkerJobRepositories, andWorkerScheduleInfoall interact with the repositories to create jobs or update their status.Besides some minor clean ups, this PR separates the logic to determine the next jobs into a new class that has no dependencies on infrastructure. It is the first in a series of changes to untangle the different responsibilities of the orchestrator to make them testable in isolation and improve overview.