Conversation
…hing - human taking over
Contributor
Reviewer's Guide by SourceryThis pull request introduces a mechanism to detect gaps in the sequence of messages for a subscriber. It adds a Sequence diagram for gap detection in CatchupServicesequenceDiagram
participant CatchupService
participant Database
CatchupService->>Database: SELECT idn FROM postevent.messages WHERE idn > currentHwm ORDER BY idn
activate Database
Database-->>CatchupService: ResultSet (message IDs)
deactivate Database
loop For each message ID in ResultSet
CatchupService->>CatchupService: Check if (actualIdn > expectedNext)
alt Gap found
CatchupService->>CatchupService: Log gap
CatchupService-->>CatchupService: break
else No gap
CatchupService->>CatchupService: Update lastContiguousIdn and expectedNext
end
end
alt lastContiguousIdn > currentHwm
CatchupService->>CatchupService: Update HWM
CatchupService->>Database: Update HWM in database
activate Database
Database-->>CatchupService: Confirmation
deactivate Database
end
CatchupService-->>CatchupService: Return gapFound
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @p14n - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a method to the test class to encapsulate the event creation logic to avoid duplication.
- The
copyEventsToMessagesmethod duplicates data - consider if this is the best approach.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| * @return true if a gap was found, false if no gaps were found | ||
| * @throws SQLException If a database error occurs | ||
| */ | ||
| public boolean hasSequenceGap(String subscriberName, long currentHwm) throws SQLException { |
Contributor
There was a problem hiding this comment.
suggestion: Method name might not clearly express its side effects.
Since this method both checks for sequence gaps and updates the HWM when contiguous messages are found, consider renaming it (or enhancing the documentation) so its dual behavior is immediately evident to future maintainers.
Suggested implementation:
/**
* Checks for gaps in the message sequence and updates the HWM to the last contiguous message.
*
* This method both detects gaps and, if contiguous messages are found, advances the HWM.
*
* @param subscriberName The name of the subscriber
* @param currentHwm The current high water mark to start checking from
* @return true if a gap was found, false if no gaps were found (and the HWM has been updated)
* @throws SQLException If a database error occurs
*/
public boolean checkAndAdvanceHwm(String subscriberName, long currentHwm) throws SQLException {Ensure that all references to the method "hasSequenceGap" in other parts of your codebase (e.g., callers and tests) are updated to "checkAndAdvanceHwm" to maintain consistency.
| @@ -81,6 +83,14 @@ private void createProcessingGap() throws Exception { | |||
| """); | |||
| } | |||
Contributor
There was a problem hiding this comment.
suggestion (testing): Edge case: Multiple gaps
Consider adding a test case with multiple gaps in the sequence to ensure the hasSequenceGap method handles them correctly. For example, a sequence like 1, 2, 4, 5, 7, 8 would have gaps at 3 and 6.
Suggested change
| } | |
| } | |
| @Test | |
| public void testHasSequenceGapWithMultipleGaps() throws Exception { | |
| // Create multiple gaps by publishing events with specific IDs. | |
| // We'll manually insert events with IDs 1, 2, 4, 5, 7, 8 (gaps at 3 and 6) | |
| log.debug("Publishing events with multiple gaps"); | |
| // Insert events with IDs: 1, 2 | |
| for (int i = 1; i <= 2; i++) { | |
| Event event = new Event( | |
| UUID.randomUUID().toString(), | |
| "test-source", | |
| "test-type", | |
| "application/json", | |
| null, | |
| i // assuming this parameter represents the event's numeric ID | |
| ); | |
| publishEvent(event); | |
| } | |
| // Insert events with IDs: 4, 5 | |
| for (int i = 4; i <= 5; i++) { | |
| Event event = new Event( | |
| UUID.randomUUID().toString(), | |
| "test-source", | |
| "test-type", | |
| "application/json", | |
| null, | |
| i | |
| ); | |
| publishEvent(event); | |
| } | |
| // Insert events with IDs: 7, 8 | |
| for (int i = 7; i <= 8; i++) { | |
| Event event = new Event( | |
| UUID.randomUUID().toString(), | |
| "test-source", | |
| "test-type", | |
| "application/json", | |
| null, | |
| i | |
| ); | |
| publishEvent(event); | |
| } | |
| // Verify that gaps are detected in the sequence | |
| boolean hasGap = hasSequenceGap(); | |
| assertTrue(hasGap, "Should detect gaps in events with multiple missing sequence numbers"); | |
| // Verify HWM was updated to the last event (ID 8) | |
| long newHwm = getCurrentHwm(SUBSCRIBER_NAME); | |
| assertEquals(8, newHwm, "HWM should be updated to the last event"); | |
| } |
|
|
||
| TODO | ||
| Create a processor | ||
| - [ ] ? |
Contributor
There was a problem hiding this comment.
issue (typo): Remove the stray question mark.
The question mark seems like leftover debugging text and should be removed for clarity.
| @@ -157,4 +157,59 @@ private void updateHwm(String subscriberName, long currentHwm, long newHwm) thro | |||
| } | |||
Contributor
There was a problem hiding this comment.
issue (complexity): Consider extracting the ResultSet loop into a helper method to reduce nesting complexity and improve readability of the hasSequenceGap method, by creating a private method that processes the result set and returns both the gap flag and the last contiguous IDN.
Consider extracting the ResultSet loop into a helper method to reduce nesting. For example, you could create a private method that processes the result set and returns both the gap flag and the last contiguous IDN:
private static class GapCheckResult {
final boolean gapFound;
final long lastContiguousIdn;
GapCheckResult(boolean gapFound, long lastContiguousIdn) {
this.gapFound = gapFound;
this.lastContiguousIdn = lastContiguousIdn;
}
}
private GapCheckResult processMessages(ResultSet rs, long currentHwm) throws SQLException {
long expectedNext = currentHwm + 1;
long lastContiguousIdn = currentHwm;
while (rs.next()) {
long actualIdn = rs.getLong("idn");
if (actualIdn > expectedNext) {
LOGGER.log(Level.INFO, "Gap found: Expected {0}, found {1} (gap of {2})",
new Object[]{expectedNext, actualIdn, actualIdn - expectedNext});
return new GapCheckResult(true, lastContiguousIdn);
}
lastContiguousIdn = actualIdn;
expectedNext = actualIdn + 1;
}
return new GapCheckResult(false, lastContiguousIdn);
}Then simplify your hasSequenceGap method by using this helper:
public boolean hasSequenceGap(String subscriberName, long currentHwm) throws SQLException {
LOGGER.log(Level.FINE, "Checking for sequence gaps after HWM {0} for subscriber {1}",
new Object[]{currentHwm, subscriberName});
String sql = "SELECT idn FROM postevent.messages WHERE idn > ? ORDER BY idn";
try (PreparedStatement stmt = connection.prepareStatement(sql)) {
stmt.setLong(1, currentHwm);
try (ResultSet rs = stmt.executeQuery()) {
GapCheckResult result = processMessages(rs, currentHwm);
if (result.lastContiguousIdn > currentHwm) {
LOGGER.log(Level.INFO, "Updating HWM from {0} to {1} for subscriber {2}",
new Object[]{currentHwm, result.lastContiguousIdn, subscriberName});
updateHwm(subscriberName, currentHwm, result.lastContiguousIdn);
}
if (!result.gapFound) {
LOGGER.log(Level.INFO, "No sequence gaps found after HWM for subscriber {0}", subscriberName);
}
return result.gapFound;
}
}
}This refactoring preserves your functionality while reducing the nesting complexity in hasSequenceGap.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Implements a cursor catchup mechanism that checks for gaps in the message sequence and updates the HWM to the last contiguous message.
New Features:
Tests: