SAK-52275 Assignments PeerReview Schedule a retry after detecting an error in reviewer allocation #14323
Conversation
st-manu
changed the title
…error in reviewer allocation
WalkthroughAdds validations, counters, mapping/pruning for submissions and assessors, creation/handling of peer-assessment items, retry scheduling with exponential backoff, coverage validation, and expanded logging and error handling in the peer-assessment execution flow. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.java📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
Files:
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughEnhances AssignmentPeerAssessmentServiceImpl with preflight validation, progress tracking, improved error handling, and retry scheduling for peer assessment execution. Adds two new helper methods for coverage validation and retry scheduling. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java`:
- Around line 626-635: scheduleRetry currently schedules a new delayed
invocation without removing any existing ones, risking duplicate retries; update
scheduleRetry to first call removeScheduledPeerReview() (the same cleanup used
by schedulePeerReview) before creating the delayed invocation and replace the
hard-coded 5-minute Instant.now().plusSeconds(300) with a configurable delay
value (e.g., read from a config property or constant) so the retry interval can
be tuned; ensure you still log the retryTime and preserve the existing exception
handling around scheduledInvocationManager.createDelayedInvocation.
- Around line 302-305: The current retry scheduling in the block guarded by
success, totalReviewsAssigned, and totalSubmissions can create an infinite loop;
modify the logic around the condition (the branch that calls
scheduleRetry(assignment)) to enforce a retry limit and backoff: add a retry
counter property on the Assignment entity (or a separate persistent/volatile
counter keyed by assignment id) that you increment each time
scheduleRetry(assignment) is invoked, stop scheduling once the counter reaches a
configurable maxRetries, and implement exponential backoff by passing/updating a
delay parameter to scheduleRetry (or computing delay from the retry count); also
add a circuit-breaker check using the same counter/state to log a final warning
and avoid further retries when structural constraints persist (use symbols:
success, totalReviewsAssigned, totalSubmissions, scheduleRetry(assignment),
assignment, opaqueContext).
🧹 Nitpick comments (1)
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java (1)
309-322: Minor: PreferStringUtils.isNotBlank()for consistency.Line 313 uses
!StringUtils.isBlank(opaqueContext)while line 118 uses the positive form. UsingStringUtils.isNotBlank()would be more readable and consistent.Additionally, this retry-on-error path has the same infinite retry concern if the error is persistent (e.g., database connectivity issues that take hours to resolve).
♻️ Suggested consistency fix
- if (!StringUtils.isBlank(opaqueContext)) { + if (StringUtils.isNotBlank(opaqueContext)) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
🔇 Additional comments (4)
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java (4)
108-143: LGTM! Comprehensive preflight validation.The added validations correctly guard against null/empty assignment IDs, missing assignments, invalid review counts, and empty submissions. Logging uses SLF4J parameterized format as per coding guidelines.
177-178: LGTM!Progress tracking correctly incremented within the submission processing loop.
281-284: LGTM!Appropriate tracking of assigned reviews with helpful debug/warn logging when a reviewer cannot be found.
590-621: LGTM! Good validation helper method.The coverage validation correctly counts non-removed reviews per submission and identifies under-reviewed submissions with appropriate logging.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...ment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java
Show resolved
Hide resolved
...ment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java (1)
228-228: Use parameterized logging instead of string concatenation.This log statement uses string concatenation which violates the coding guidelines for SLF4J logging. As per coding guidelines, use parameterized logging.
Suggested fix
- log.error("AssignmentPeerAssessmentServiceImpl: found a peer assessment with an invalid session id: " + p.getId().getSubmissionId()); + log.error("AssignmentPeerAssessmentServiceImpl: found a peer assessment with an invalid submission id: {}", p.getId().getSubmissionId());Note: The message also contains a typo ("session id" should be "submission id").
🧹 Nitpick comments (2)
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java (2)
309-322: Re-fetching assignment in catch block may fail again.If the original exception was thrown by
assignmentService.getAssignment(opaqueContext)at line 124, the retry scheduling will also fail since it calls the same method at line 314. Consider caching the assignment reference before the try block or handling this scenario differently.Suggested approach
+ Assignment assignment = null; try { if (StringUtils.isBlank(opaqueContext)) { log.error("Peer assessment job called with null or empty assignment ID"); return; } //for group assignments, we need to have a user ID, otherwise, an exception is thrown: - Assignment assignment = assignmentService.getAssignment(opaqueContext); + assignment = assignmentService.getAssignment(opaqueContext); // ... rest of the code } catch (Exception e) { log.error("Error during peer assessment assignment process for assignment {}: {}", opaqueContext, e.getMessage(), e); - try { - if (!StringUtils.isBlank(opaqueContext)) { - Assignment assignment = assignmentService.getAssignment(opaqueContext); - if (assignment != null && assignment.getAllowPeerAssessment() && !assignment.getDraft()) { - log.debug("Scheduling retry for failed peer assessment assignment: {}", opaqueContext); - scheduleRetry(assignment); - } - } - } catch (Exception retryException) { - log.error("Failed to schedule retry for assignment {}: {}", opaqueContext, retryException.getMessage()); - } + if (assignment != null && assignment.getAllowPeerAssessment() && !assignment.getDraft()) { + log.debug("Scheduling retry for failed peer assessment assignment: {}", opaqueContext); + scheduleRetry(assignment); + } } finally {
623-635: Consider removing existing scheduled invocations before scheduling retry.Unlike
schedulePeerReview()which callsremoveScheduledPeerReview()first (line 81), this method doesn't remove existing invocations before scheduling a new one. If the retry also fails and schedules another retry, this could lead to duplicate scheduled invocations accumulating.Suggested fix
private void scheduleRetry(Assignment assignment) { try { + // Remove any existing scheduled invocations first to prevent duplicates + removeScheduledPeerReview(assignment.getId()); // Schedule retry in 5 minutes Instant retryTime = Instant.now().plusSeconds(300); log.debug("Scheduling peer assessment retry for assignment {} at {}", assignment.getId(), retryTime); scheduledInvocationManager.createDelayedInvocation(retryTime, "org.sakaiproject.assignment.api.AssignmentPeerAssessmentService", assignment.getId()); } catch (Exception e) { log.error("Failed to schedule retry for assignment {}: {}", assignment.getId(), e.getMessage(), e); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
🔇 Additional comments (7)
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java (7)
107-121: LGTM! Good defensive validation.The early validation for blank assignment ID and the use of parameterized SLF4J logging aligns with best practices. The counters for tracking submissions and reviews are well-initialized.
124-143: LGTM! Thorough input validation.The null checks for assignment and submissions, along with the validation of
numOfReviews, provide robust guard clauses that prevent processing invalid data.
177-179: LGTM!Counter increment and debug logging are correctly placed within the submission eligibility check.
281-286: LGTM!Appropriate counter tracking and logging when assigning reviewers, with a clear warning when no available reviewer can be found.
290-308: Consider retry logic for partial assignment failures.The retry is only scheduled when
totalReviewsAssigned == 0, but there could be cases where some reviews were assigned (totalReviewsAssigned > 0) yetvalidatePeerAssessmentCoveragereturnsfalsedue to incomplete coverage. In such scenarios, no retry is scheduled.Is this intentional? If partial assignment failures should also trigger retries, consider:
- log.info("Peer assessment assignment completed for {}: {} submissions processed, {} reviews assigned, success: {}", opaqueContext, totalSubmissions, totalReviewsAssigned, success); - if (!success && totalReviewsAssigned == 0 && totalSubmissions > 0) { - log.warn("No reviews were assigned for assignment {} with {} submissions. Scheduling retry.", opaqueContext, totalSubmissions); + log.info("Peer assessment assignment completed for {}: {} submissions processed, {} reviews assigned, success: {}", opaqueContext, totalSubmissions, totalReviewsAssigned, success); + if (!success && totalSubmissions > 0) { + log.warn("Peer assessment incomplete for assignment {} with {} submissions and {} reviews assigned. Scheduling retry.", opaqueContext, totalSubmissions, totalReviewsAssigned); scheduleRetry(assignment); }
306-308: LGTM!Appropriate warning when assignment configuration prevents peer assessment processing.
598-602: Note the same pattern on line 545 of this method: WhilegetRemoved()returns aBooleanobject that could theoretically be null, this defensive check pattern is used consistently throughout the codebase (e.g., line 545, AssignmentAction.java, AssignmentEntityProvider.java) and is mitigated by the database column constraint (nullable = false) and field initialization (Boolean.FALSE). If you wish to apply defensive null-safety for consistency, apply it uniformly across both line 545 and 599, and potentially the broader codebase pattern.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java`:
- Around line 314-326: The catch block currently calls scheduleRetry(assignment)
unconditionally, bypassing the retry-count check in shouldScheduleRetry(), so
change it to call shouldScheduleRetry(...) with the same argument used elsewhere
(e.g., the assignment or opaqueContext) and only invoke
scheduleRetry(assignment) when shouldScheduleRetry(...) returns true; also log
when a retry is skipped and preserve existing exception logging for
retryException.
🧹 Nitpick comments (1)
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java (1)
643-677: Consider renaming to reflect side effects.The method name
shouldScheduleRetrysuggests a read-only check, but it also increments the retry counter and persists it to the database. Consider renaming tocheckAndIncrementRetryCountor similar to make the side effects explicit.Additionally, extracting property keys and
maxRetriesto constants would improve maintainability:♻️ Suggested improvements
+ private static final String RETRY_COUNT_KEY_PREFIX = "peerAssessmentRetryCount_"; + private static final String LAST_ATTEMPT_KEY_PREFIX = "peerAssessmentLastAttempt_"; + private static final int MAX_PEER_ASSESSMENT_RETRIES = 5; + /** - * Checks if a retry should be scheduled based on retry count limit + * Checks retry eligibility and increments the retry counter if eligible. + * `@return` true if retry should be scheduled, false if max retries reached */ - private boolean shouldScheduleRetry(Assignment assignment, int totalSubmissions) { - String retryCountKey = "peerAssessmentRetryCount_" + assignment.getId(); - String lastAttemptKey = "peerAssessmentLastAttempt_" + assignment.getId(); + private boolean checkAndIncrementRetryCount(Assignment assignment, int totalSubmissions) { + String retryCountKey = RETRY_COUNT_KEY_PREFIX + assignment.getId(); + String lastAttemptKey = LAST_ATTEMPT_KEY_PREFIX + assignment.getId(); try { // Get current retry count from assignment properties String retryCountStr = assignment.getProperties().get(retryCountKey); int retryCount = retryCountStr != null ? Integer.parseInt(retryCountStr) : 0; - int maxRetries = 5; - if (retryCount >= maxRetries) { - log.warn("Assignment {} has reached maximum retry attempts ({}), stopping retries", assignment.getId(), maxRetries); + if (retryCount >= MAX_PEER_ASSESSMENT_RETRIES) { + log.warn("Assignment {} has reached maximum retry attempts ({}), stopping retries", assignment.getId(), MAX_PEER_ASSESSMENT_RETRIES); return false; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (8)
assignment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java (8)
104-139: LGTM! Well-structured early validation guards.The fail-fast approach with clear logging at each validation step improves debuggability. Parameterized logging is correctly used throughout.
173-174: LGTM!Counter increment and debug logging are appropriate.
277-281: LGTM!Proper tracking of assigned reviews with informative debug logging.
224-224: LGTM!Correctly uses parameterized logging per coding guidelines.
286-309: LGTM! Properly addresses previous review concerns.The retry logic now correctly:
- Limits retries via
shouldScheduleRetry(max 5 attempts)- Clears retry counters on success
- Validates existing coverage when no new items are created
594-641: LGTM! Thorough coverage validation with proper edge case handling.The method correctly handles structural limitations (single submission, insufficient reviewers) and provides clear logging for debugging under-reviewed submissions.
682-700: LGTM! Properly addresses previous review concerns.The method now:
- Removes existing scheduled invocations before scheduling (prevents duplicates)
- Implements exponential backoff (30s base, 10-minute cap)
- Logs scheduling details appropriately
702-730: LGTM!Efficient implementation that only persists changes when retry properties existed, with appropriate exception handling.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...ment/impl/src/java/org/sakaiproject/assignment/impl/AssignmentPeerAssessmentServiceImpl.java
Show resolved
Hide resolved
1 participant
https://sakaiproject.atlassian.net/browse/SAK-52275
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.