Development: Optimize local git repository cleanup
#9322
Conversation
github-actions
bot
added
the
server
...a/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java
Outdated
Show resolved
Hide resolved
krusche
changed the title
Development: Optimise Local Git Repository CleanupDevelopment: Optimize local git repository cleanup
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…-performance-for-git-cleanup
WalkthroughThe changes involve updates to a Gradle configuration file to expand the exclusion of tasks during execution, as well as modifications to Java repository and service classes. This includes the removal of specific query methods from the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range comments (2)
src/test/java/de/tum/cit/aet/artemis/exercise/programming/ProgrammingExerciseTestService.java (2)
Line range hint
2363-2363: Rename test method to reflect the updated method being testedThe test method
automaticCleanupGitRepositories()now callscleanupGitWorkingCopiesOnArtemisServer(). To maintain clarity and consistency, consider renaming the test method toautomaticCleanupGitWorkingCopies()to accurately reflect the functionality being tested.Apply this diff to rename the test method:
-void automaticCleanupGitRepositories() { +void automaticCleanupGitWorkingCopies() {
Line range hint
2363-2387: Add assertions to verify the behavior ofcleanupGitWorkingCopiesOnArtemisServer()The test method currently lacks assertions to validate the behavior of
cleanupGitWorkingCopiesOnArtemisServer(). Without assertions, the test may not effectively detect regressions or unintended side effects. Consider adding appropriate assertions or verifying interactions with mocked services to ensure the method functions as expected.
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (5)
- .idea/runConfigurations/_template__of_Gradle.xml (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseRepository.java (0 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java (4 hunks)
- src/test/java/de/tum/cit/aet/artemis/exercise/programming/ProgrammingExerciseTestService.java (1 hunks)
Files not reviewed due to no reviewable changes (1)
- src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseRepository.java
Additional context used
Path-based instructions (3)
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.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_importssrc/main/java/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.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_importssrc/test/java/de/tum/cit/aet/artemis/exercise/programming/ProgrammingExerciseTestService.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
Additional comments not posted (3)
.idea/runConfigurations/_template__of_Gradle.xml (1)
7-7: Optimized Exclusions Enhance Test Run EfficiencyThe addition of
-x nodeSetup -x npmSetup -x npmInstall -x webappto thescriptParameterseffectively skips unnecessary tasks during test runs, leading to improved execution times. This aligns with the PR's objective to optimize the build process.src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (2)
12-13: Imports added for pagination supportThe addition of
PageandPageableimports is necessary for the new paginated query method.
169-181: Ensure efficient query execution with indexesThe query filters data based on
pe.dueDateandpe.exerciseGroup.exam.endDate. To optimize performance for large datasets, verify that indexes exist on these date fields.Run the following script to check for indexes on the relevant date columns:
Verification successful
Indexes are in place, but entity field definitions are unclear
The necessary indexes for optimizing the query performance are present in the database schema:
exam_end_dateon the exam tableexercise_due_dateon the exercise tableThese indexes align with the fields used in the query (
pe.dueDateandpe.exerciseGroup.exam.endDate), which should ensure efficient query execution for large datasets.However, it's worth noting that the actual field definitions for
dueDatein the Exercise class andendDatein the Exam class were not directly visible in our search. This could be due to inheritance, getter/setter methods, or alternative naming conventions. While this doesn't affect the query performance, it might be beneficial for code maintainability to review how these fields are defined and accessed in the entity classes.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if indexes exist on pe.dueDate and pe.exerciseGroup.exam.endDate fields. # Expected: Index annotations on dueDate and endDate fields to improve query performance. # Search for @Index annotations on the dueDate field in ProgrammingExercise entity rg --type java "@Index.*dueDate" src/main/java/ # Search for @Index annotations on the endDate field in Exam entity rg --type java "@Index.*endDate" src/main/java/Length of output: 99
Script:
#!/bin/bash # Description: Further investigation of index definitions # Search for @Table annotations with indexes attribute echo "Searching for @Table annotations with indexes:" rg --type java "@Table\([^)]*indexes\s*=" src/main/java/ # Search for database migration scripts echo "Searching for database migration scripts:" fd -e sql -e xml -e yaml -e yml | grep -i "migration\|flyway\|liquibase" # Search for custom index creation methods echo "Searching for custom index creation methods:" rg --type java "createIndex|addIndex" src/main/java/ # Broader search for potential entity classes echo "Searching for potential entity classes:" rg --type java "class (ProgrammingExercise|Exam)" src/main/java/Length of output: 10526
Script:
#!/bin/bash # Description: Search for index creations in Liquibase changelogs and locate entity classes # Search for index creations on dueDate and endDate in Liquibase changelogs echo "Searching for index creations in Liquibase changelogs:" rg -i "createIndex.*(?:due_date|end_date)" src/main/resources/config/liquibase/ # Search for ProgrammingExercise and Exam entity classes echo "Searching for ProgrammingExercise and Exam entity classes:" rg --type java "@Entity.*class (ProgrammingExercise|Exam)" src/main/java/ # If entity classes are not found, search for classes extending a common base class or implementing an interface echo "Searching for potential entity classes with inheritance:" rg --type java "class (ProgrammingExercise|Exam).*extends.*" src/main/java/Length of output: 4143
Script:
#!/bin/bash # Description: Confirm field existence and index naming # Check for dueDate field in Exercise class echo "Searching for dueDate field in Exercise class:" rg --type java "class Exercise.*\{[\s\S]*?dueDate.*?;" src/main/java/ # Check for endDate field in Exam class echo "Searching for endDate field in Exam class:" rg --type java "class Exam.*\{[\s\S]*?endDate.*?;" src/main/java/ # Double-check index names in Liquibase changelogs echo "Verifying index names in Liquibase changelogs:" rg -i "createIndex.*(?:exercise.*due_date|exam.*end_date)" src/main/resources/config/liquibase/Length of output: 1193
...it/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java
Outdated
Show resolved
Hide resolved
...a/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java
Outdated
Show resolved
Hide resolved
...a/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (2)
- src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java (4 hunks)
Additional context used
Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.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_importssrc/main/java/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.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 not posted (3)
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (3)
12-13: Imports added are appropriateThe addition of
PageandPageableimports is necessary to support pagination in the new repository method.
169-181: Previous review comments are still applicableThe issues highlighted in the previous review comments regarding potential
NullPointerExceptionwhen accessingpe.exerciseGroup.exam.endDateand the confusion with parameter namesearliestDateandlatestDateare still present in this code segment.
169-181: Method implementation aligns with objectivesThe new method
findRepositoryUrisForGitCleanupByRecentDueDateOrRecentExamEndDateeffectively retrieves repository URIs based on recent due dates or exam end dates, which should optimize the cleanup process as intended.
...a/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java
Show resolved
Hide resolved
...it/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java
Outdated
Show resolved
Hide resolved
…-performance-for-git-cleanup
julian-christl
dismissed stale reviews from JohannesStoehr, Strohgelaender, and coderabbitai[bot]
via
d21fc6e
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java (5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.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_importssrc/main/java/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.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 (8)
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (2)
12-13: Remove unnecessary importsThe imports
org.springframework.data.domain.Pageandorg.springframework.data.domain.Pageableare not necessary unless pagination is used in the code. Please ensure these imports are required; otherwise, consider removing them to keep the code clean.Run the following script to check if
PageandPageableare used elsewhere in this file:
12-13: Ensure compliance with coding guidelinesPlease verify that the changes adhere to the provided coding guidelines, specifically:
- naming:CamelCase: Method and variable names should use CamelCase.
- practices:avoid_transactions: Avoid unnecessary transactions in repository methods.
- java:avoid_star_imports: Ensure imports are explicit and avoid using wildcard imports.
Run the following script to check for wildcard imports:
Also applies to: 167-181
✅ Verification successful
No Wildcard Imports Found
No wildcard imports are present in the repository, ensuring compliance with the
java:avoid_star_importsguideline.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no wildcard imports are used. # Test: Search for wildcard imports in Java files. grep -rn 'import .*.\*;' src/main/java/Length of output: 41
Script:
#!/bin/bash # Description: Accurately verify that no wildcard imports are used. # Test: Search for wildcard imports in Java files with a refined grep pattern. grep -rn '^import\s\+[^;]*\.\*;' src/main/java/Length of output: 49
src/main/java/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java (6)
6-6: Import statement is necessary and correctThe import of
java.net.URISyntaxExceptionis required for proper exception handling in thedeleteLocalRepositoryByUriStringmethod.
20-21: Pagination imports are appropriateThe imports of
PageandPageablefromorg.springframework.data.domainare correctly added for handling pagination in repository queries.
31-31: Import ofVcsRepositoryUriis validIncluding
VcsRepositoryUriis essential for constructing repository URIs when deleting local repositories.
91-91: Initiate cleanup of Git working copiesThe invocation of
cleanupGitWorkingCopiesOnArtemisServer()correctly starts the cleanup process for local Git repositories.
94-94: Proper exception loggingThe added log statement ensures that any exceptions occurring during
cleanupGitWorkingCopiesOnArtemisServer()are logged with the stack trace, aiding in debugging.
129-145: Logic incleanStudentParticipationsRepositoriesis soundThe method efficiently retrieves and processes repository URIs in batches to delete local student repositories. The pagination logic and loop structure are correctly implemented.
...it/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java
Outdated
Show resolved
Hide resolved
...it/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java
Outdated
Show resolved
Hide resolved
...it/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java
Show resolved
Hide resolved
...a/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java
Show resolved
Hide resolved
...a/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java
Show resolved
Hide resolved
...a/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…-performance-for-git-cleanup
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java (5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.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_importssrc/main/java/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.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/repository/ProgrammingExerciseStudentParticipationRepository.java (1)
Learnt from: julian-christl PR: ls1intum/Artemis#9322 File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:0-0 Timestamp: 2024-09-17T12:06:23.646Z Learning: In Artemis, an `ExerciseGroup` always has an associated `Exam`, so `exerciseGroup.exam` is never null.
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (2)
12-13: LGTM: Appropriate imports addedThe new imports for
PageandPageablefromorg.springframework.data.domainare correctly added to support the new paginated query method.
167-180: 🛠️ Refactor suggestionConsider optimizing the query and improving naming
Method name:
The current method name is quite long. Consider a more concise name that still captures the essence of the operation.Date range comparison:
The date range comparisons can be simplified using the BETWEEN keyword for better readability.Parameter naming:
The parameter names 'earliestDate' and 'latestDate' might be confusing as they are used in reverse order in the conditions. Consider using 'startDate' and 'endDate' for clarity.JOIN operations:
Verify if LEFT JOINs are necessary for pe.exerciseGroup and eg.exam. If these relationships are always present, using INNER JOINs might improve performance.Here's a suggested refactoring:
- @Query(""" - SELECT participation.repositoryUri - FROM ProgrammingExerciseStudentParticipation participation - JOIN TREAT (participation.exercise AS ProgrammingExercise) pe - LEFT JOIN pe.exerciseGroup eg - LEFT JOIN eg.exam exam - WHERE participation.repositoryUri IS NOT NULL - AND ( - (pe.dueDate IS NOT NULL AND pe.dueDate BETWEEN :earliestDate AND :latestDate) - OR (eg IS NOT NULL AND exam IS NOT NULL AND exam.endDate BETWEEN :earliestDate AND :latestDate) - ) - """) - Page<String> findRepositoryUrisByRecentDueDateOrRecentExamEndDate(@Param("earliestDate") ZonedDateTime earliestDate, @Param("latestDate") ZonedDateTime latestDate, - Pageable pageable); + @Query(""" + SELECT participation.repositoryUri + FROM ProgrammingExerciseStudentParticipation participation + JOIN TREAT (participation.exercise AS ProgrammingExercise) pe + LEFT JOIN pe.exerciseGroup eg + LEFT JOIN eg.exam exam + WHERE participation.repositoryUri IS NOT NULL + AND ( + (pe.dueDate IS NOT NULL AND pe.dueDate BETWEEN :startDate AND :endDate) + OR (eg IS NOT NULL AND exam IS NOT NULL AND exam.endDate BETWEEN :startDate AND :endDate) + ) + """) + Page<String> findRepositoryUrisByDateRange(@Param("startDate") ZonedDateTime startDate, @Param("endDate") ZonedDateTime endDate, + Pageable pageable);To verify if LEFT JOINs are necessary, we can check if there are exercises without exercise groups or exams:
If this query returns results, it confirms that LEFT JOINs are necessary.
...a/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.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 (3)
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (3)
175-176: Good use of BETWEEN for date range checksThe use of the BETWEEN operator for date range checks is a good practice. It's clear, concise, and generally performs well. Keep this approach for date range filtering.
179-180: Effective use of pagination for large result setsThe method returns a
Page<String>, which is an excellent choice for handling potentially large result sets. This approach allows for efficient memory usage and improved performance when dealing with a large number of repository URIs.
170-172: 🛠️ Refactor suggestionConsider optimizing joins if exerciseGroup and exam are always present
The query uses LEFT JOINs for
pe.exerciseGroupandeg.exam. If you're certain that these relationships always exist for valid records, consider using INNER JOINs instead. This could potentially improve query performance.However, if there's a possibility that these relationships might not always exist, the current LEFT JOINs are appropriate.
To verify if INNER JOINs can be safely used, we can check if there are any programming exercises without exercise groups or exams:
If these patterns are always present, consider updating the query to use INNER JOINs:
-JOIN TREAT (participation.exercise AS ProgrammingExercise) pe -LEFT JOIN pe.exerciseGroup eg -LEFT JOIN eg.exam exam +JOIN TREAT (participation.exercise AS ProgrammingExercise) pe +JOIN pe.exerciseGroup eg +JOIN eg.exam exam✅ Verification successful
LEFT JOINs are appropriate for the current data model
Based on the analysis of the codebase, the current use of LEFT JOINs in the query is appropriate and should be maintained. Here's why:
- The relationships between ProgrammingExercise, ExerciseGroup, and Exam are indirect and hierarchical.
- Not all ProgrammingExercises are guaranteed to be part of an ExerciseGroup or Exam.
- The codebase shows examples of accessing ExerciseGroup and Exam through ProgrammingExercise, but these relationships may not always exist.
For example, in
ExamDateService.java, we see:return getExamProgrammingExerciseUnlockDate(exercise.getExerciseGroup().getExam());This suggests that a ProgrammingExercise can be associated with an ExerciseGroup, which in turn can be associated with an Exam. However, these associations are not mandatory for all ProgrammingExercises.
Therefore, using LEFT JOINs ensures that all ProgrammingExercises are included in the query results, even if they don't have associated ExerciseGroups or Exams. Changing to INNER JOINs could potentially exclude valid ProgrammingExercises from the query results.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for programming exercises without exercise groups or exams ast-grep --lang java --pattern 'class ProgrammingExercise { $$$ ExerciseGroup exerciseGroup; $$$ }' ast-grep --lang java --pattern 'class ExerciseGroup { $$$ Exam exam; $$$ }'Length of output: 439
Script:
#!/bin/bash # Check for ProgrammingExercise classes without ExerciseGroup, excluding problematic file fd --type f --extension java --exclude 'src/test/resources/test-data/repository-export/EncodingISO_8559_1.java' | \ xargs -I {} ast-grep --lang java --pattern 'class ProgrammingExercise { $$$ ExerciseGroup exerciseGroup; $$$ }' # Check for ExerciseGroup classes without Exam, excluding problematic file fd --type f --extension java --exclude 'src/test/resources/test-data/repository-export/EncodingISO_8559_1.java' | \ xargs -I {} ast-grep --lang java --pattern 'class ExerciseGroup { $$$ Exam exam; $$$ }'Length of output: 279729
Script:
#!/bin/bash # Search for ProgrammingExercise class definition echo "Searching for ProgrammingExercise class:" rg "class ProgrammingExercise" --type java --glob '!**/EncodingISO_8559_1.java' # Search for ExerciseGroup within ProgrammingExercise echo "\nSearching for ExerciseGroup within ProgrammingExercise:" rg "class ProgrammingExercise.*?ExerciseGroup" --type java --glob '!**/EncodingISO_8559_1.java' -U # Search for Exam within ExerciseGroup echo "\nSearching for Exam within ExerciseGroup:" rg "class ExerciseGroup.*?Exam" --type java --glob '!**/EncodingISO_8559_1.java' -ULength of output: 10895
Script:
#!/bin/bash # Examine ProgrammingExercise class echo "Examining ProgrammingExercise class:" rg "class ProgrammingExercise.*?ExerciseGroup|class ProgrammingExercise.*?Exam" src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExercise.java -U # Check Exercise class echo "\nChecking Exercise class:" rg "class Exercise.*?ExerciseGroup|class Exercise.*?Exam" --type java -U # Search for usage of ProgrammingExercise, ExerciseGroup, and Exam together echo "\nSearching for usage of ProgrammingExercise, ExerciseGroup, and Exam together:" rg "ProgrammingExercise.*?ExerciseGroup.*?Exam" --type java -ULength of output: 1245
...it/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java
Show resolved
Hide resolved
…-performance-for-git-cleanup
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (1)
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java (1)
Line range hint
2359-2387: Avoid direct database access in test methodAccording to the coding guidelines (
avoid_db_access: true), tests should not directly access the database. In theautomaticCleanupGitRepositoriestest method, several repository methods are called, such asprogrammingExerciseRepository.save(exercise);. Please refactor the test to use static mocks and fixed data instead.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseRepository.java (0 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java (5 hunks)
- src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java (1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseRepository.java
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.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_importssrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.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
🔇 Additional comments (4)
src/main/java/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java (4)
101-127: Refactored method improves clarity and maintainabilityThe
cleanupGitWorkingCopiesOnArtemisServermethod has been refactored to delegate specific cleanup tasks to helper methods. This enhances readability and adheres to the single responsibility principle.
105-107: 🧹 Nitpick (assertive)Variable names might be confusing
The variables
latestDateandearliestDatemight cause confusion becauseearliestDateis actually older thanlatestDate. For better readability, consider renaming them tostartDateandendDate.
146-152:⚠️ Potential issueCatch broader exceptions during repository deletion
Currently, only
URISyntaxExceptionis caught in thedeleteLocalRepositoryByUriStringmethod. IfgitService.deleteLocalRepository(vcsRepositoryUrl)can throw other exceptions (e.g.,GitAPIException,IOException), it's advisable to catch these to prevent the cleanup process from failing.Modify the catch block to handle general exceptions:
} catch (URISyntaxException e) { log.error("Cannot create URI for repositoryUri: {}", uri, e); +} catch (Exception e) { + log.error("Failed to delete local repository for URI: {}", uri, e); }Likely invalid or redundant comment.
51-52: 🛠️ Refactor suggestionConsider making 'STUDENT_PARTICIPATION_CLEANUP_BATCH_SIZE' configurable
To enhance flexibility, consider externalizing the
STUDENT_PARTICIPATION_CLEANUP_BATCH_SIZEto application properties. This allows for easier adjustments without code changes.Apply this diff to make the batch size configurable:
-private static final int STUDENT_PARTICIPATION_CLEANUP_BATCH_SIZE = 500; +@Value("${artemis.cleanup.student-participation-batch-size:500}") +private int studentParticipationCleanupBatchSize;And update the usage:
-Pageable pageable = Pageable.ofSize(STUDENT_PARTICIPATION_CLEANUP_BATCH_SIZE); +Pageable pageable = Pageable.ofSize(studentParticipationCleanupBatchSize);Likely invalid or redundant comment.
...a/de/tum/cit/aet/artemis/programming/service/AutomaticProgrammingExerciseCleanupService.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
Show resolved
Hide resolved
krusche
deleted the
chore/programming-exercises/improve-query-performance-for-git-cleanup
branch
Checklist
General
Server
Motivation and Context
The two deleted database calls were highly inefficient and took > 30 seconds on production.
I also modified the Gradle run configuration template as currently with every test execution npmInstall and other tasks get executed for no reason
Description
We actually only need the repository URI for deleting the local repository. Everything else is either irrelevant or only for logging purposes. Thus, I created one query searching both conditions (exercises and exams) at once and only returning the URI.
To have finer control, I reworked it as a paginated run. The batch size is up for debate.
Steps for Testing
Local Test only
Prerequisites:
repos) (be careful if unrelated copies already exist. Don't get confused or delete them beforehand).AutomaticProgrammingExerciseCleanupService.cleanupGitWorkingCopiesOnArtemisServer()and update theearliestDateto be before your exercise/exam due dates (i.e.earliestDateandlatestDateshould include the due dates).artemis.scheduling.programming-exercises-cleanup-timeto a reasonable schedule for you to test or modify the scheduled parameter of thecleanup()method so you don't have to wait.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
Performance Tests
Test Coverage
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores