Development: Fix issue when high number of items are added to the queue in a short period of time
#9876
Conversation
github-actions
bot
added
server
WalkthroughThe Changes
Possibly related PRs
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.7.0)src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. 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
🧹 Outside diff range and nitpick comments (3)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (3)
113-114: Improved thread safety in collection handlingThe change from streams to direct collection instantiation is a good improvement for concurrent scenarios. The comment effectively explains the rationale.
Consider adding synchronization if you need to perform multiple operations atomically on these collections.
Also applies to: 118-119
123-124: LGTM: Improved stability for course-specific queriesCreating stable snapshots before filtering is a good practice. Consider adding index-based filtering if the number of jobs per course becomes very large.
Also applies to: 128-129
168-171: Consider optimizing multiple iterationsWhile the change improves stability, the method performs multiple iterations over the queue:
- First to check if job exists
- Then to collect jobs to remove
Consider combining these operations into a single pass.
public void cancelBuildJob(String buildJobId) { - List<BuildJobQueueItem> queuedJobs = new ArrayList<>(queue); - if (queuedJobs.stream().anyMatch(job -> Objects.equals(job.id(), buildJobId))) { - List<BuildJobQueueItem> toRemove = new ArrayList<>(); - for (BuildJobQueueItem job : queuedJobs) { - if (Objects.equals(job.id(), buildJobId)) { - toRemove.add(job); - } - } - queue.removeAll(toRemove); + List<BuildJobQueueItem> queuedJobs = new ArrayList<>(queue); + List<BuildJobQueueItem> toRemove = queuedJobs.stream() + .filter(job -> Objects.equals(job.id(), buildJobId)) + .toList(); + if (!toRemove.isEmpty()) { + queue.removeAll(toRemove); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.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
🔇 Additional comments (5)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/SharedQueueManagementService.java (5)
8-8: LGTM: Import addition is appropriate
The addition of java.util.Collection import supports the improved collection handling throughout the service.
133-134: LGTM: Consistent implementation pattern
Also applies to: 138-139
210-211: LGTM: Consistent collection handling
The changes maintain consistency with the improved collection handling pattern used throughout the service.
Also applies to: 222-223
233-234: LGTM: Consistent implementation
Also applies to: 248-249, 263-264, 271-272
148-150: Verify the impact of publicSshKey addition
While the collection handling changes are good, let's verify the publicSshKey usage across the codebase.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
publicSshKey addition is properly integrated
The publicSshKey field is consistently used across the codebase:
- Correctly passed through BuildAgentInformation constructor in SharedQueueProcessingService
- Properly handled in GitPublickeyAuthenticatorService for SSH authentication
- Verified in BuildAgentSshAuthenticationIntegrationTest
- Consistently propagated in SharedQueueManagementService
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BuildAgentInformation constructor usage
ast-grep --pattern 'new BuildAgentInformation($$$)'
# Search for publicSshKey references
rg "publicSshKey" --type java
Length of output: 3160
github-actions
bot
added
the
buildagent
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/SharedQueueProcessingService.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
| List<BuildJobQueueItem> processingJobsList = new ArrayList<>(processingJobs.values()); | ||
| return processingJobsList.stream().filter(job -> Objects.equals(job.buildAgent().memberAddress(), memberAddress)).toList(); |
There was a problem hiding this comment.
💡 Codebase verification
Potential concurrent modification issues found in SharedQueueProcessingService
The verification revealed another instance in SharedQueueProcessingService that directly streams from a Hazelcast collection:
processingJobs.getAll(runningBuildJobIdsAfterGracePeriod).values().stream().toList();This pattern is susceptible to the same concurrent modification issue. The locations that need attention:
SharedQueueProcessingService.java: Line 363 (already fixed in this PR)SharedQueueProcessingService.java: Direct streaming fromprocessingJobs.getAll(...).values()needs similar defensive copying
Other usages of .values().stream() in the codebase are safe as they operate on regular Java collections, not Hazelcast distributed collections.
🔗 Analysis chain
LGTM! The change addresses concurrent modification issues.
The modification to create a new ArrayList before streaming is a good approach to prevent IllegalStateException when the underlying Hazelcast map is modified concurrently. This aligns with the PR objective of fixing issues during rapid queue updates.
For better performance, consider using a more memory-efficient approach:
- List<BuildJobQueueItem> processingJobsList = new ArrayList<>(processingJobs.values());
- return processingJobsList.stream().filter(job -> Objects.equals(job.buildAgent().memberAddress(), memberAddress)).toList();
+ return processingJobs.values().stream()
+ .filter(job -> Objects.equals(job.buildAgent().memberAddress(), memberAddress))
+ .collect(Collectors.toCollection(ArrayList::new));Let's verify if similar patterns exist in the codebase that might need the same fix:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct streaming on Hazelcast collections
ast-grep --pattern 'IMap<$_, $_> $map = $_;
$$$
$map.values().stream().$$$'
# Search for potential concurrent access patterns
rg -A 2 'values\(\)\.stream\(\)'
Length of output: 9569
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
Steps for Testing
Prerequisites:
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
Performance Review
Code Review
Manual Tests
Summary by CodeRabbit
Bug Fixes
New Features