Iris: Improve code quality
#9494
+553
−320
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,12 +2,7 @@ | |||||||||||||||||||
|
|
||||||||||||||||||||
| import static de.tum.cit.aet.artemis.core.config.Constants.PROFILE_IRIS; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import java.util.List; | ||||||||||||||||||||
| import java.util.function.Consumer; | ||||||||||||||||||||
| import java.util.function.Function; | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -17,27 +12,11 @@ | |||||||||||||||||||
| import org.springframework.context.annotation.Profile; | ||||||||||||||||||||
| import org.springframework.stereotype.Service; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import de.tum.cit.aet.artemis.iris.exception.IrisException; | ||||||||||||||||||||
| import de.tum.cit.aet.artemis.iris.service.pyris.dto.PyrisPipelineExecutionDTO; | ||||||||||||||||||||
| import de.tum.cit.aet.artemis.iris.service.pyris.dto.PyrisPipelineExecutionSettingsDTO; | ||||||||||||||||||||
| import de.tum.cit.aet.artemis.iris.service.pyris.dto.status.PyrisStageDTO; | ||||||||||||||||||||
| import de.tum.cit.aet.artemis.iris.service.pyris.job.PyrisJob; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Service responsible for executing the various Pyris pipelines in a type-safe manner. | ||||||||||||||||||||
|
|
@@ -53,34 +32,18 @@ public class PyrisPipelineService { | |||||||||||||||||||
|
|
||||||||||||||||||||
| private final PyrisJobService pyrisJobService; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @Value("${server.url}") | ||||||||||||||||||||
| private String artemisBaseUrl; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| public PyrisPipelineService(PyrisConnectorService pyrisConnectorService, PyrisJobService pyrisJobService) { | ||||||||||||||||||||
| this.pyrisConnectorService = pyrisConnectorService; | ||||||||||||||||||||
| this.pyrisJobService = pyrisJobService; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Executes a pipeline on Pyris, identified by the given name and variant. | ||||||||||||||||||||
| * The pipeline execution is tracked by a unique job token, which is created for each new job automatically. | ||||||||||||||||||||
| * The caller must provide a mapper function to take this job token and produce a {@code PyrisJob} object to be registered. | ||||||||||||||||||||
MichaelOwenDyer marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+45
to
+46
There was a problem hiding this comment. 🧹 Nitpick (assertive) Improve clarity in JavaDoc comments Consider rephrasing the JavaDoc comments for better clarity:
Apply this diff to enhance the JavaDoc comments: - * The pipeline execution is tracked by a unique job token, which is created for each new job automatically.
+ * The pipeline execution is tracked by a unique job token, which is automatically created for each new job.
- * The caller must provide a mapper function to take this job token and produce a {@code PyrisJob} object to be registered.
+ * The caller must provide a mapper function that takes this job token and produces a {@code PyrisJob} object to be registered.📝 Committable suggestion
Suggested change
Comment on lines
+45
to
+46
There was a problem hiding this comment. Clarify Parameter Description in Documentation In the Javadoc, the description of the Suggested change: - * The caller must provide a mapper function to take this job token and produce a {@code PyrisJob} object to be registered.
+ * The caller must provide a mapper function that takes the generated job ID and produces a {@code PyrisJob} object to be registered.📝 Committable suggestion
Suggested change
|
||||||||||||||||||||
| * The caller must additionally provide a mapper function to create the concrete DTO type for this pipeline from the base DTO. | ||||||||||||||||||||
| * The status of the pipeline execution is updated via a consumer that accepts a list of stages. This method will | ||||||||||||||||||||
| * call the consumer with the initial stages of the pipeline execution. Later stages will be sent back from Pyris, | ||||||||||||||||||||
|
|
@@ -89,11 +52,12 @@ public PyrisPipelineService(PyrisConnectorService pyrisConnectorService, PyrisJo | |||||||||||||||||||
| * | ||||||||||||||||||||
| * @param name the name of the pipeline to be executed | ||||||||||||||||||||
| * @param variant the variant of the pipeline | ||||||||||||||||||||
| * @param jobFunction a function from job ID to job. Creates a new {@code PyrisJob} which will be registered in Hazelcast | ||||||||||||||||||||
| * @param dtoMapper a function to create the concrete DTO type for this pipeline from the base execution DTO | ||||||||||||||||||||
| * @param statusUpdater a consumer of stages to send status updates while the pipeline is being prepared | ||||||||||||||||||||
|
There was a problem hiding this comment. 🧹 Nitpick (assertive) Enhance parameter descriptions in JavaDoc To improve clarity and consistency in the parameter descriptions, consider the following revisions:
Apply this diff to update the parameter descriptions: - * @param jobFunction a function from job ID to job. Creates a new {@code PyrisJob} which will be registered in Hazelcast
+ * @param jobFunction a function that accepts a job ID and returns a {@code PyrisJob} to be registered in Hazelcast
- * @param dtoMapper a function to create the concrete DTO type for this pipeline from the base execution DTO
+ * @param dtoMapper a function that transforms the base execution DTO into a concrete DTO type for this pipeline
- * @param statusUpdater a consumer of stages to send status updates while the pipeline is being prepared
+ * @param statusUpdater a consumer that accepts a list of stages to provide status updates during pipeline preparation📝 Committable suggestion
Suggested change
|
||||||||||||||||||||
| */ | ||||||||||||||||||||
| public void executePipeline(String name, String variant, Function<String, PyrisJob> jobFunction, Function<PyrisPipelineExecutionDTO, Object> dtoMapper, | ||||||||||||||||||||
| Consumer<List<PyrisStageDTO>> statusUpdater) { | ||||||||||||||||||||
|
Comment on lines
+59
to
+60
There was a problem hiding this comment. 🛠️ Refactor suggestion Consider Simplifying Method Parameters The |
||||||||||||||||||||
| // Define the preparation stages of pipeline execution with their initial states | ||||||||||||||||||||
| // There will be more stages added in Pyris later | ||||||||||||||||||||
| var preparing = new PyrisStageDTO("Preparing", 10, null, null); | ||||||||||||||||||||
|
|
@@ -102,6 +66,8 @@ public void executePipeline(String name, String variant, String jobToken, Functi | |||||||||||||||||||
| // Send initial status update indicating that the preparation stage is in progress | ||||||||||||||||||||
| statusUpdater.accept(List.of(preparing.inProgress(), executing.notStarted())); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| String jobToken = pyrisJobService.registerJob(jobFunction); | ||||||||||||||||||||
|
There was a problem hiding this comment. Add Error Handling for Job Registration The call to Apply this diff to include error handling: + String jobToken;
+ try {
+ jobToken = pyrisJobService.registerJob(jobFunction);
+ } catch (Exception e) {
+ log.error("Failed to register job", e);
+ statusUpdater.accept(List.of(preparing.error("Failed to register job"), executing.notStarted()));
+ return;
+ }📝 Committable suggestion
Suggested change
|
||||||||||||||||||||
|
|
||||||||||||||||||||
MichaelOwenDyer marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
| var baseDto = new PyrisPipelineExecutionDTO(new PyrisPipelineExecutionSettingsDTO(jobToken, List.of(), artemisBaseUrl), List.of(preparing.done())); | ||||||||||||||||||||
| var pipelineDto = dtoMapper.apply(baseDto); | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -123,106 +89,4 @@ public void executePipeline(String name, String variant, String jobToken, Functi | |||||||||||||||||||
| statusUpdater.accept(List.of(preparing.error("An internal error occurred"), executing.notStarted())); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Approved with a minor suggestion for improved readability.
The changes look good overall. The method name change from
oftofromimproves consistency withCompetencyJolDTO, and the updated logic correctly usesCompetencyJolDTO.from(). The null checks are maintained, preserving the existing behavior.Consider splitting the long line for better readability:
This formatting maintains the logic while improving code readability.