Assessment: Fix sporadically delayed score calculation
#10289
Conversation
github-actions
bot
added
server
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
WalkthroughThe Changes
Sequence Diagram(s)Suggested labels
Suggested reviewers
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/assessment/service/ParticipantScoreScheduleService.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| ✨ 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: 0
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ParticipantScoreScheduleService.java (2)
224-235: Consider adding null checks for method parameters.While the exerciseId parameter is annotated with @NotNull, the method implementation should still validate all parameters to ensure robustness.
private void scheduleTask(Long exerciseId, Long participantId, Instant resultLastModified, Long resultIdToBeDeleted) { + if (exerciseId == null || participantId == null || resultLastModified == null) { + throw new IllegalArgumentException("exerciseId, participantId, and resultLastModified must not be null"); + } final var participantScoreId = new ParticipantScoreId(exerciseId, participantId);
343-343: Consider using computeIfPresent for atomic removal.When dealing with concurrent maps, using atomic operations can help prevent race conditions.
- scheduledTasks.remove(new ParticipantScoreId(exerciseId, participantId)); + scheduledTasks.computeIfPresent(new ParticipantScoreId(exerciseId, participantId), (key, value) -> null);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ParticipantScoreScheduleService.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/assessment/service/ParticipantScoreScheduleService.java
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: server-tests-postgres
- GitHub Check: server-tests-mysql
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ParticipantScoreScheduleService.java (3)
72-72: LGTM! Good choice using ConcurrentHashMap with the new key type.The change from using Integer to ParticipantScoreId as the map key improves type safety while maintaining thread safety through ConcurrentHashMap.
469-470: LGTM! Well-designed record class.The ParticipantScoreId record is a perfect use case for Java records, providing:
- Immutable composite key
- Auto-generated equals/hashCode for map operations
- Clear semantic meaning
1-471: Verify the impact of ParticipantScoreId changes on serialization.Since this service is part of a distributed system (as indicated by the broker reference in comments), we should verify that the new ParticipantScoreId is properly handled in serialization/deserialization scenarios.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for serialization-related configurations or implementations rg -l "implements.*Serializable|@JsonSerialize|@JsonDeserialize"Length of output: 6169
ParticipantScoreId Serialization Impact Verified
After reviewing the repository, it appears that the ParticipantScoreId record is used solely as an internal key (in a ConcurrentHashMap for scheduling tasks) and is not involved in any serialization/deserialization scenarios (e.g., no JSON annotations or Serializable implementations). The searched serialization configurations are present in other parts of the codebase (mostly DTOs), and there’s no evidence that ParticipantScoreId is transmitted over the broker or included in any API messages. Therefore, the changes to ParticipantScoreId have no adverse impact on serialization.
magaupp
changed the title
Assessment: Fix uncommon outdated score calculationAssessment: Fix missing score calculation
src/main/java/de/tum/cit/aet/artemis/assessment/service/ParticipantScoreScheduleService.java
Outdated
Show resolved
Hide resolved
magaupp
dismissed stale reviews from az108, simonbohnen, and coderabbitai[bot]
via
342d40f
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ParticipantScoreScheduleService.java (1)
471-472: LGTM! Well-designed record type for composite key.The
ParticipantScoreIdrecord is a good choice for the composite key:
- Immutable by design (record type)
- Clear semantic meaning
- Built-in equals/hashCode implementation
Consider adding validation for non-null fields if the IDs are never expected to be null.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ParticipantScoreScheduleService.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/assessment/service/ParticipantScoreScheduleService.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: server-style
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ParticipantScoreScheduleService.java (3)
73-73: LGTM! Improved type safety with custom key type.The change from
Map<Integer, ScheduledFuture<?>>toMap<ParticipantScoreId, ScheduledFuture<?>>eliminates potential hash collisions by using a proper composite key type.
225-237: LGTM! Task scheduling logic correctly uses the new key type.The implementation properly creates and uses the
ParticipantScoreIdfor task management. The code maintains thread safety by usingConcurrentHashMapand proper synchronization when canceling and rescheduling tasks.
345-345: LGTM! Consistent cleanup in finally block.The task cleanup in the finally block correctly uses the new
ParticipantScoreIdtype, ensuring proper resource management even in case of exceptions.
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
|
The Hash table internally uses |
magaupp
changed the title
Assessment: Fix missing score calculationAssessment: Fix sporadically delayed score calculation
magaupp
dismissed stale reviews from ole-ve, HanyangXu0508, kevinfischer4, SindiBuklaji, and coderabbitai[bot]
via
9b621e5
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ParticipantScoreScheduleService.java (2)
473-474: LGTM! Well-designed record type.The
ParticipantScoreIdrecord is a good choice for representing the composite key. It's immutable and provides automatic implementations ofequals(),hashCode(), andtoString().Consider adding validation to ensure neither field is null:
-public record ParticipantScoreId(Long exerciseId, Long participantId) { +public record ParticipantScoreId(@NotNull Long exerciseId, @NotNull Long participantId) {
9-10: Consider using more specific imports.The code imports both
ArraysandMapfromjava.util. For better maintainability and clarity, consider using specific imports for collection types.-import java.util.Arrays; -import java.util.Map; +import java.util.Arrays; +import java.util.concurrent.ConcurrentMap;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ParticipantScoreScheduleService.java(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/assessment/service/ParticipantScoreScheduleService.java
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ParticipantScoreScheduleService.java (3)
58-59: LGTM! Documentation improvements.The added comments provide valuable context about potential network outages and the role of the cron job as a fallback mechanism.
74-74: LGTM! Type safety improvement.Replacing
IntegerwithParticipantScoreIdas the map key type enhances type safety and makes the code more maintainable by preventing hash collisions.
228-229: LGTM! Task scheduling logic improvements.The implementation correctly uses the new
ParticipantScoreIdrecord for task identification, which resolves the issue of incorrect task cancellations due to hash collisions.Also applies to: 233-233, 238-238, 347-347
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
Checklist
General
Server
Motivation and Context
ParticipantScoreScheduleService tries to detect duplicate task submissions by identifying tasks by a hash built from its exercise id and its participant id. This approach, however, does not handle hash collisions.
The result is that sometimes tasks get canceled by other, different tasks and a required update of the ParticipantScore gets dropped.
These missed results get picked up by the
@Scheduledcronjob that runs every minute.Description
The tasks get identified by their exercise id and participant id without taking the hash. The Hashmap implementation internally uses the hash, but also compares the entire key to ensure the correct identity of the entry.
Fixes flaky tests in ParticipantScoreIntegrationTest.
Steps for Testing
Prerequisites:
Before / After assessment:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Summary by CodeRabbit
Summary by CodeRabbit