Programming exercises: Fix an issue with missing schedules to combine template commits
#10215
Conversation
improve logging and add a REST endpoint to review current schedules
github-actions
bot
added
tests
server
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 PMD (7.8.0)src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseLifecycleService.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| src/main/java/de/tum/cit/aet/artemis/core/service/ScheduleService.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| src/main/java/de/tum/cit/aet/artemis/core/config/TaskSchedulingConfiguration.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185|
WalkthroughThis pull request introduces several significant changes across multiple services and components in the Artemis project. The primary focus is on enhancing task scheduling, improving method naming conventions, and refining repository and service interactions. Key modifications include updating method signatures in scheduling services, introducing new record types for task management, renaming methods in the GitService for clarity, and adding a new admin schedule resource. The changes aim to improve code readability, type safety, and provide more descriptive task scheduling mechanisms. Changes
Suggested Labels
Suggested Reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (16)
src/main/java/de/tum/cit/aet/artemis/core/config/TaskSchedulingConfiguration.java (1)
26-26: Make pool size configurable via properties.Consider making the pool size configurable through application properties to allow for environment-specific tuning.
Apply this diff to make the pool size configurable:
+ @Value("${artemis.scheduling.pool-size:4}") + private int poolSize; @Bean(name = "taskScheduler") public TaskScheduler taskScheduler() { log.debug("Creating Task Scheduler "); var scheduler = new ThreadPoolTaskScheduler(); - scheduler.setPoolSize(4); + scheduler.setPoolSize(poolSize);Add the following to your
application.properties:# Configure the size of the task scheduler thread pool (default: 4) artemis.scheduling.pool-size=4src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.java (2)
Line range hint
161-173: Consider simplifying the stream operations for better readability.The current stream operation chain can be simplified by combining the filter and map operations into the collector.
- var files = gitService.getFilesAndFolders(repository).entrySet().stream().filter(entry -> entry.getValue() == FileType.FILE).map(Map.Entry::getKey).toList(); + var files = gitService.getFilesAndFolders(repository).entrySet().stream() + .filter(entry -> entry.getValue() == FileType.FILE) + .map(Map.Entry::getKey) + .toList();
265-269: Consider improving readability of stream operations.The complex stream operations can be made more readable by breaking them into multiple lines and using meaningful variable names.
- var repoFiles = gitService.getFilesAndFolders(repository).entrySet().stream().filter(entry -> entry.getValue() == FileType.FILE).map(Map.Entry::getKey).toList(); - - Map<String, File> templateRepoFiles = gitService.getFilesAndFolders(templateRepository).entrySet().stream().filter(entry -> entry.getValue() == FileType.FILE) - .collect(Collectors.toMap(entry -> entry.getKey().toString(), Map.Entry::getKey)); + var repoFiles = gitService.getFilesAndFolders(repository).entrySet().stream() + .filter(entry -> entry.getValue() == FileType.FILE) + .map(Map.Entry::getKey) + .toList(); + + Map<String, File> templateRepoFiles = gitService.getFilesAndFolders(templateRepository).entrySet().stream() + .filter(entry -> entry.getValue() == FileType.FILE) + .collect(Collectors.toMap( + entry -> entry.getKey().toString(), + Map.Entry::getKey + ));src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseScheduleService.java (3)
145-145: LGTM! Consider enhancing the task description.The change to
scheduleExerciseTaskwith a descriptive parameter improves task tracking. Consider making the description more specific by including the exercise ID: "build modeling clusters after due date for exercise #{exerciseId}".- scheduleService.scheduleExerciseTask(exercise, ExerciseLifecycle.DUE, () -> buildModelingClusters(exercise).run(), "build modeling clusters after due date"); + scheduleService.scheduleExerciseTask(exercise, ExerciseLifecycle.DUE, () -> buildModelingClusters(exercise).run(), + String.format("build modeling clusters after due date for exercise #%d", exercise.getId()));
163-164: LGTM! Consider enhancing the task description for consistency.The change to
scheduleExerciseTaskis good. For consistency with the previous suggestion and better tracking, consider making the description more specific.- scheduleService.scheduleExerciseTask(exercise, ExerciseLifecycle.BUILD_COMPASS_CLUSTERS_AFTER_EXAM, () -> buildModelingClusters(exercise).run(), - "build modeling clusters after exam"); + scheduleService.scheduleExerciseTask(exercise, ExerciseLifecycle.BUILD_COMPASS_CLUSTERS_AFTER_EXAM, () -> buildModelingClusters(exercise).run(), + String.format("build modeling clusters after exam for exercise #%d", exercise.getId()));
Line range hint
191-204: Enhance error handling and logging in buildModelingClusters.The error handling in the
buildModelingClustersmethod could be improved to provide more context and better logging.private Runnable buildModelingClusters(ModelingExercise exercise) { Long modelingExerciseId = exercise.getId(); return () -> { SecurityUtils.setAuthorizationObject(); + log.debug("Starting to build modeling clusters for exercise #{}", modelingExerciseId); try { Optional<ModelingExercise> modelingExercise = modelingExerciseRepository.findById(modelingExerciseId); if (modelingExercise.isEmpty()) { throw new EntityNotFoundException("modeling exercise not found with id " + modelingExerciseId); } compassService.build(modelingExercise.get()); + log.debug("Successfully built modeling clusters for exercise #{}", modelingExerciseId); } catch (EntityNotFoundException ex) { - log.error("Modeling exercise with id {} is no longer available in database for use in scheduled task.", modelingExerciseId); + log.error("Failed to build modeling clusters: exercise #{} no longer exists", modelingExerciseId, ex); + } + catch (Exception ex) { + log.error("Unexpected error while building modeling clusters for exercise #{}", modelingExerciseId, ex); } }; }src/main/java/de/tum/cit/aet/artemis/core/service/ScheduleService.java (2)
105-153: Scheduled logging and cleanup every 15 seconds.
Continuously removing non-running tasks is beneficial to avoid stale entries. Consider reviewing 15-second intervals for potential performance overhead in high-traffic environments.
232-240: Multiple-task scheduling at different times.
Good handling of multipleTuple<ZonedDateTime, Runnable>entries for the same lifecycle. Ensure no concurrency issues if tasks overlap in runtime.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseScheduleService.java (1)
348-348: Fetching participations.
Ensure large queries with.findWithSubmissionsAndTeamStudentsByExerciseIdremain performant. Index usage might be beneficial.src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminScheduleResource.java (3)
22-24: Update JavaDoc comment to match the class purpose.The JavaDoc comment incorrectly states "REST controller for getting the audit events" but this controller is for getting scheduled events.
-* REST controller for getting the audit events. +* REST controller for getting scheduled exercise events.
28-28: Follow REST API best practices for base URL.The base URL should not end with a trailing slash as it can lead to double-slash issues.
-@RequestMapping("api/admin/") +@RequestMapping("api/admin")
43-48: Consider adding response status documentation.The endpoint implementation looks good, but could benefit from explicit documentation of possible response status codes using @ApiResponse annotations.
src/main/java/de/tum/cit/aet/artemis/exercise/domain/ExerciseLifecycle.java (1)
11-22: Extract magic number to a constant.The 15 seconds offset is a magic number that should be extracted to a named constant for better maintainability and clarity.
+private static final int RELEASE_PREPARATION_OFFSET_SECONDS = 15; SHORTLY_BEFORE_RELEASE { @Override public ZonedDateTime getDateFromExercise(Exercise exercise) { - return exercise.getReleaseDate() != null ? exercise.getReleaseDate().minusSeconds(15) : null; + return exercise.getReleaseDate() != null ? exercise.getReleaseDate().minusSeconds(RELEASE_PREPARATION_OFFSET_SECONDS) : null; }src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizScheduleService.java (1)
107-109: Consider extracting task name to a constant.The task name "calculate all quiz results" could be extracted to a constant to maintain consistency and enable reuse.
+private static final String TASK_NAME_CALCULATE_RESULTS = "calculate all quiz results"; public void scheduleCalculateAllResults(QuizExercise quizExercise) { if (quizExercise.getDueDate() != null && !quizExercise.isQuizEnded()) { scheduleService.scheduleExerciseTask(quizExercise, ExerciseLifecycle.DUE, - new Tuple<>(quizExercise.getDueDate().plusSeconds(5), () -> quizSubmissionService.calculateAllResults(quizExercise.getId())), "calculate all quiz results"); + new Tuple<>(quizExercise.getDueDate().plusSeconds(5), () -> quizSubmissionService.calculateAllResults(quizExercise.getId())), TASK_NAME_CALCULATE_RESULTS);src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseScheduleServiceTest.java (2)
374-376: Improve line formatting for better readability.Consider reformatting the method call to avoid line wrapping:
- verify(scheduleService, timeout(TIMEOUT_MS)).scheduleParticipationTask(eq(participationIndividualDueDate), eq(ParticipationLifecycle.BUILD_AND_TEST_AFTER_DUE_DATE), any(), - any()); + verify(scheduleService, timeout(TIMEOUT_MS)).scheduleParticipationTask( + eq(participationIndividualDueDate), + eq(ParticipationLifecycle.BUILD_AND_TEST_AFTER_DUE_DATE), + any(), + any() + );
565-566: Improve line formatting for better readability.Consider reformatting the method call to avoid line wrapping:
- verify(scheduleService, after(SCHEDULER_TASK_TRIGGER_DELAY_MS).never()).scheduleExerciseTask(eq(programmingExercise), eq(ExerciseLifecycle.DUE), any(Runnable.class), - any()); + verify(scheduleService, after(SCHEDULER_TASK_TRIGGER_DELAY_MS).never()).scheduleExerciseTask( + eq(programmingExercise), + eq(ExerciseLifecycle.DUE), + any(Runnable.class), + any() + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
build.gradle(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/NotificationScheduleService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/TaskSchedulingConfiguration.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/ScheduleService.java(8 hunks)src/main/java/de/tum/cit/aet/artemis/core/util/Tuple.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminScheduleResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/domain/ExerciseLifecycle.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseLifecycleService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseScheduleService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/JavaTemplateUpgradeService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseRepositoryService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseScheduleService.java(16 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/AbstractVersionControlService.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizScheduleService.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/core/service/TitleCacheEvictionServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/GitServiceTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseScheduleServiceTest.java(11 hunks)src/test/java/de/tum/cit/aet/artemis/programming/RepositoryIntegrationTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/AbstractVersionControlService.java
✅ Files skipped from review due to trivial changes (5)
- build.gradle
- src/test/java/de/tum/cit/aet/artemis/core/service/TitleCacheEvictionServiceTest.java
- src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java
- src/test/java/de/tum/cit/aet/artemis/programming/GitServiceTest.java
- src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.java
🧰 Additional context used
📓 Path-based instructions (16)
src/main/java/de/tum/cit/aet/artemis/exercise/domain/ExerciseLifecycle.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/util/Tuple.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/JavaTemplateUpgradeService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseRepositoryService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/config/TaskSchedulingConfiguration.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseScheduleService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/programming/RepositoryIntegrationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizScheduleService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminScheduleResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseLifecycleService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/communication/service/NotificationScheduleService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseScheduleService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseScheduleServiceTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/main/java/de/tum/cit/aet/artemis/core/service/ScheduleService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
📓 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/JavaTemplateUpgradeService.java (1)
Learnt from: magaupp
PR: ls1intum/Artemis#9074
File: src/main/java/de/tum/in/www1/artemis/service/programming/TemplateUpgradePolicyService.java:35-35
Timestamp: 2024-11-12T12:51:46.554Z
Learning: The `defaultRepositoryUpgradeService` in the `TemplateUpgradePolicyService` is used as a default for all non-JAVA languages, with specialized services added based on testing outcomes if necessary.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Analyse
- GitHub Check: Build .war artifact
🔇 Additional comments (53)
src/main/java/de/tum/cit/aet/artemis/core/config/TaskSchedulingConfiguration.java (1)
27-27: LGTM!Explicitly calling
initialize()is a good practice to ensure the scheduler is properly set up before use.src/main/java/de/tum/cit/aet/artemis/programming/service/JavaTemplateUpgradeService.java (1)
106-106: LGTM! Method name change improves clarity.The change from
listFilestogetFilesis consistent with similar updates across the codebase and better reflects the method's purpose.src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java (3)
323-326: LGTM! Parameter name better reflects its type.The parameter rename from
targetUrltotargetUriis more accurate as it represents aVcsRepositoryUriobject.
Line range hint
1018-1029: LGTM! Method name and documentation improvements.The change from
listFilesAndFolderstogetFilesAndFoldersbetter reflects the method's purpose. Documentation updates accurately describe the method's behavior, including important details about caching and symbolic link handling.
1051-1057: LGTM! Method name and documentation improvements.The change from
listFilestogetFilesis consistent with the naming pattern. Documentation clarifies that an empty list is returned for empty repositories.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseRepositoryService.java (2)
327-327: LGTM! Method name updated for consistency.Updated to use the renamed
getFilesmethod from GitService.
361-361: LGTM! Method name updated for consistency.Updated to use the renamed
getFilesmethod from GitService.src/test/java/de/tum/cit/aet/artemis/programming/RepositoryIntegrationTest.java (1)
347-347: LGTM! Test mock updated to match renamed method.Updated the mock to use the renamed
getFilesAndFoldersmethod from GitService.src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.java (3)
100-100: LGTM! Method name change is consistent.The renaming from
listFilesAndFolderstogetFilesAndFoldersmaintains the method's functionality while improving naming consistency.
161-161: LGTM! Method name change is consistent.The renaming from
listFilesAndFolderstogetFilesAndFoldersaligns with the codebase's naming conventions.
265-267: LGTM! Method name changes are consistent.The renaming from
listFilesAndFolderstogetFilesAndFoldersmaintains consistency across the codebase.src/main/java/de/tum/cit/aet/artemis/core/service/ScheduleService.java (13)
4-4: No issues with updated imports.
The newly introduced imports are specific and non-duplicative. There's no use of wildcard imports.Also applies to: 6-7, 9-13, 18-21, 26-26, 28-33
56-67: Good use of Java records for typed keys.
Introducing dedicated records (ExerciseLifecycleKey,ParticipationLifecycleKey,ScheduledTaskName, andScheduledExerciseEvent) increases code clarity and type safety.
68-68: ConcurrentMap usage looks appropriate.
UsingConcurrentHashMapfor tracking scheduled tasks is suitable. Verify concurrency correctness for atomic removals in repeated tasks (e.g.,removeIf).Also applies to: 71-72
73-75: Initialization of TaskScheduler with virtual threads.
UsingsetVirtualThreads(true)is modern and can handle concurrency well, but ensure this environment supports virtual threads (Java 19+).Also applies to: 80-88
89-103: Pagination logic for scheduled exercise events.
Flattening and sorting scheduled tasks before pagination is clear. Great approach to returning aPage.
155-157: Helper methods for adding/removing scheduled tasks.
Small, focused methods adhere to single responsibility. This is maintainable and consistent with best practices.Also applies to: 160-161, 164-166, 169-171, 174-176
185-192: Exercise scheduling with descriptive naming.
IncludingString nameimproves traceability when diagnosing scheduled tasks.
202-210: QuizExercise scheduling.
Similar approach as standard exercises. Ensure all quiz-specific validations (e.g., batch constraints) are handled.
212-224: Single-task scheduling with time.
Clear separation of scheduling logic for a singleTuple<ZonedDateTime, Runnable>. The method is self-documenting and flexible.
249-254: Participation-specific scheduling.
This method reads well. Confirm that cancellation logic properly aligns with newly introduced schedule calls.
266-276: Lifecycle cancellation logic.
Ensuring all possible tasks for an exercise are canceled prevents extraneous scheduling. Consider concurrency if tasks complete just as they’re canceled.
289-294: Participation cancellation logic.
Matches exercise-level logic. The finer granularity is beneficial for cleanup.
314-317: Global cleanup in testing.
Clearing all maps is a pragmatic approach for test environments. Minimal risk of side effects.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseScheduleService.java (15)
42-42: New import for ProfileService.
No issues; consistent with the DI approach used elsewhere.
100-101: ProfileService field injection.
Adheres to constructor injection. Clear separation of environment checks from scheduling logic.
104-104: Constructor refactoring.
ReplacingEnvironmentwithProfileServiceclarifies the logic for environment selection, removing direct reliance on profile strings.Also applies to: 107-107, 124-124
136-136: Skipping scheduling if dev profile is active.
This condition is reversed from the comment text (“only execute on production”), but the logic is correct:if (isDevActive()) return;.
298-300: Scheduling template commit combination.
Executes a commit combination task shortly before release date. The naming is explicit.
307-307: Due date scheduling with local VCS checks.
This diversified path for local VCS is a good approach. Validation: confirm that partial scheduling in local mode is intended.Also applies to: 309-325
331-332: Scheduling build & test after due date.
Method named concisely. Looks consistent with the scheduling approach of other lifecycles.
359-360: Scheduling participation with individual due dates.
This is a sensible extension for custom deadlines. Good decomposition.
382-382: Locking repositories and updating student results.
This block differentiates local VCS from remote usage gracefully. The breakdown of tasks is consistent with single responsibility.Also applies to: 384-405
420-420: Exam participation scheduling.
Ensure robust error handling forEntityNotFoundExceptionduring the exam conduction.
447-449: Unlocking exam repositories.
Checking local VCS again is consistent. The user is properly informed via logs.
456-460: Rescheduling at runtime for extended working times.
This is a helpful fallback mechanism in case of crashes. Good clarity in the logs.
467-468: Build and test after exam’s buildAndTestStudentSubmissionsAfterDueDate.
Similar approach as normal exercises. well integrated.
479-479: Combining template commits in Git.
Error handling is well-structured. The logs provide clear traceability if an exception occurs.Also applies to: 485-485, 488-488
1021-1022: Detailed error logging in concurrency.
Providing participation ID in logs is beneficial for debugging. Ensure none of these logs leak sensitive data.src/main/java/de/tum/cit/aet/artemis/core/util/Tuple.java (2)
3-4: Serializable import introduced.
MakingTupleserializable is beneficial for distributed systems or caching usage.
8-11: Renamed generic parameters and fields.
Switching toF/Sandfirst/secondclarifies usage. Documenting them in Javadoc is consistent with best practices.src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminScheduleResource.java (1)
31-35: LGTM! Constructor injection is properly used.The service dependency is marked as final and injected through constructor, following best practices.
src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseLifecycleService.java (2)
104-107: LGTM! Tuple access methods are more readable.The change from x()/y() to first()/second() improves code readability by using more descriptive method names.
50-50: Consider keeping debug level for frequent scheduling events.Changing log level from debug to info might lead to log pollution as scheduling events are frequent. Consider keeping it at debug level unless there's a specific monitoring requirement.
src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizScheduleService.java (1)
92-93: LGTM! Task name parameter improves observability.The addition of a descriptive task name "quiz start" enhances monitoring and debugging capabilities.
src/main/java/de/tum/cit/aet/artemis/communication/service/NotificationScheduleService.java (2)
117-123: LGTM! Improved task identification with descriptive parameter.The change from
scheduleTasktoscheduleExerciseTaskwith the added descriptive parameter enhances task tracking and debugging capabilities.
158-164: LGTM! Consistent task identification pattern.The change maintains consistency with the previous update, using a clear descriptive parameter for assessment notifications.
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseScheduleServiceTest.java (6)
345-346: LGTM! Test updated to verify new method signature.Test correctly verifies the updated
scheduleParticipationTaskmethod with the additional parameter.
396-400: LGTM! Comprehensive test coverage.Test effectively verifies both participation and exercise task scheduling, including negative cases.
419-423: LGTM! Consistent test verification pattern.Test maintains consistency with previous test cases while verifying the updated method signatures.
514-518: LGTM! Thorough verification of both positive and negative cases.Test effectively uses Mockito's never() verification to ensure certain tasks are not scheduled.
525-529: LGTM! Proper verification of multiple scheduling calls.Test effectively uses Mockito's times() verification to ensure correct number of scheduling calls.
547-548: LGTM! Clear verification of specific scheduling scenario.Test effectively verifies the expected scheduling behavior for individual due dates.
src/main/java/de/tum/cit/aet/artemis/core/config/TaskSchedulingConfiguration.java
Show resolved
Hide resolved
SimonEntholzer
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
SindiBuklaji
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
changed the title
Programming exercise: Fix an issue with missing schedules to combine template commitsProgramming exercises: Fix an issue with missing schedules to combine template commits
krusche
dismissed stale reviews from SindiBuklaji, coderabbitai[bot], sawys777, zagemello, N0W0RK, SimonEntholzer, Cathy0123456789, BBesrour, vinceclifford, HawKhiem, and Feras797
via
32a1f48
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/main/java/de/tum/cit/aet/artemis/core/service/ScheduleService.java (5)
73-87: Consider making the TaskScheduler configuration more configurable.While the initialization is correct, consider extracting the pool size and virtual threads configuration to application properties for better flexibility.
+@Value("${schedule.pool-size:1}") +private int poolSize; + +@Value("${schedule.virtual-threads:true}") +private boolean useVirtualThreads; private final TaskScheduler taskScheduler; public ScheduleService(...) { // ... ThreadPoolTaskScheduler scheduler = new ThreadPoolTaskScheduler(); - scheduler.setPoolSize(1); - scheduler.setVirtualThreads(true); + scheduler.setPoolSize(poolSize); + scheduler.setVirtualThreads(useVirtualThreads); scheduler.initialize(); this.taskScheduler = scheduler; }
89-179: Excellent implementation of task monitoring and cleanup!The periodic task monitoring implementation is well-documented and effectively handles cleanup of completed tasks. The use of debug logging is appropriate for monitoring purposes.
However, consider extracting the logging interval to a configuration property for flexibility.
+@Value("${schedule.monitoring.interval-seconds:15}") +private int monitoringIntervalSeconds; @EventListener(ApplicationReadyEvent.class) public void startup() { - taskScheduler.scheduleAtFixedRate(() -> { + taskScheduler.scheduleAtFixedRate(() -> { // ... existing code ... - }, Duration.ofSeconds(15)); + }, Duration.ofSeconds(monitoringIntervalSeconds)); }
218-218: Consider using Set.of for immutable set creation.Based on the past review comments, the use of
new HashSet<>(List.of(scheduledTask))is intentional to support task removal. However, consider adding a code comment to explain this design decision.- addScheduledExerciseTasks(exercise, lifecycle, new HashSet<>(List.of(scheduledTask)), name); + // Using mutable HashSet to support task removal in cleanup operations + addScheduledExerciseTasks(exercise, lifecycle, new HashSet<>(List.of(scheduledTask)), name);
279-282: Consider handling empty Optional more explicitly.The
ifPresentusage could be more explicit about what happens when the Optional is empty.- participationLifecycleService.scheduleTask(participation, lifecycle, task) - .ifPresent(scheduledTask -> addScheduledParticipationTask(participation, lifecycle, new HashSet<>(List.of(scheduledTask)), name)); + var scheduledTaskOpt = participationLifecycleService.scheduleTask(participation, lifecycle, task); + if (scheduledTaskOpt.isEmpty()) { + log.debug("Failed to schedule task {} for participation {}", name, participation.getId()); + return; + } + addScheduledParticipationTask(participation, lifecycle, new HashSet<>(List.of(scheduledTaskOpt.get())), name);
340-345: Add test-only annotation to clearAllTasks method.Since this method is for testing purposes only, consider adding the
@VisibleForTestingannotation.+import com.google.common.annotations.VisibleForTesting; +@VisibleForTesting public void clearAllTasks() { // ... existing code ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/de/tum/cit/aet/artemis/core/config/TaskSchedulingConfiguration.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/ScheduleService.java(8 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseLifecycleService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseScheduleService.java(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/core/config/TaskSchedulingConfiguration.java
- src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseLifecycleService.java
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseScheduleService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/service/ScheduleService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
- GitHub Check: Mend Security Check
🔇 Additional comments (5)
src/main/java/de/tum/cit/aet/artemis/core/service/ScheduleService.java (1)
56-67: Well-structured record declarations following Java best practices!The introduction of records improves type safety and reduces boilerplate code. The records are appropriately scoped and follow the single responsibility principle.
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseScheduleService.java (4)
42-42: LGTM! Clean dependency injection implementation.The ProfileService integration follows best practices with constructor injection and immutable field declaration.
Also applies to: 100-100, 102-124
136-140: LGTM! Improved profile check using dedicated service.The change from environment check to ProfileService improves encapsulation and maintainability.
479-480: LGTM! Enhanced logging with better context and consistency.The logging improvements follow best practices:
- Appropriate log levels (debug/error)
- Consistent message format
- Helpful context for troubleshooting
Also applies to: 488-489, 1021-1023
359-360: Add error handling for invalid individual due dates.The method assumes individual due dates are valid. Consider adding validation and error handling for null or invalid dates.
Run this script to check for potential null date issues:
Also applies to: 361-378
✅ Verification successful
I'll run additional checks to verify if there is any error handling (e.g. null checks) for individual due dates in the affected file. Please execute the following script:
Once these results are available, we can determine whether additional error handling for invalid individual due dates is required.
The existing null check in the calling method already handles invalid individual due dates.
- In
scheduleParticipationTasks, the code verifies that both the exercise due date andparticipation.getIndividualDueDate()are non-null before invokingscheduleParticipationWithIndividualDueDate.- This pre-check effectively cancels scheduled tasks when an individual due date is absent or invalid, so no additional error handling is required in the method itself.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for potential null date handling in similar methods rg -A 5 'getIndividualDueDate.*\b(if|assert).*null'Length of output: 53
Script:
#!/bin/bash # Check for any usage of getIndividualDueDate and its surrounding context in the file echo "Context for getIndividualDueDate in ProgrammingExerciseScheduleService.java:" rg -n -C 5 "getIndividualDueDate" src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseScheduleService.java # Check the scheduleParticipationWithIndividualDueDate method context to see if any error handling exists echo "Context for scheduleParticipationWithIndividualDueDate method:" rg -n -C 5 "scheduleParticipationWithIndividualDueDate" src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseScheduleService.java # Search for any null validation or error handling (if statements that check for null) around individual due date operations echo "Searching for null checks near individual due date handling:" rg -n -C 5 "if.*null" src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseScheduleService.javaLength of output: 15759
|
One e2e test related to locked repositories is failing, I need to look into this first, before we can merge this PR |
| */ | ||
| public record Tuple<X, Y>(X x, Y y) { | ||
| public record Tuple<F, S>(F first, S second) implements Serializable { |
There was a problem hiding this comment.
Since we're already changing this i'd suggest renaming it to Pair to make it consistent with conventions of other programming languages. A pair has inherently a first and second member. Tuples should support arbitrarily many members.
Checklist
General
Server
Motivation and Context
The scheduled task "combine template commits" currently does not work
Description
The issue was an overlap of two scheduled tasks during the release of an exercise. I separated those and did some additional changes:
NOTE: in the future, we want to avoid combining template commits at all and rather create only one commit when creating the student repo, however until we have this functionality, we need this bugfix
Steps for Testing
Prerequisites:
Also check, if this works when you import an exercise (either directly or via zip file) and when you edit an exercise shortly before the release date.
Note: the exam mode should not be affected, but testing whether combine template commits here would be nice as well
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Summary by CodeRabbit
Release Notes
New Features
SHORTLY_BEFORE_RELEASEinExerciseLifecycle.Improvements
listFiles→getFiles).Technical Updates
EnvironmentwithProfileServicein some configuration methods.Tuplerecord.Performance
These changes primarily focus on improving code clarity, scheduling mechanisms, and overall system configuration.