Conversation
Contributor
Reviewer's Guide by SourceryThis pull request introduces a catchup mechanism to handle gaps in event sequences. It includes the implementation of a CatchupServer to fetch events within a specified range, database setup changes, and associated tests. Sequence diagram for Catchup MechanismsequenceDiagram
participant PC as Persistent Consumer
participant PR as Processor
participant CC as Catchup Client
participant CS as Catchup Server
PC->>PR: New event received (idn)
PR->>PR: Check CHWM (Contiguous High Water Mark)
alt CHWM != idn-1
PR->>CC: Trigger catchup mechanism
CC->>CS: Request batch of messages (CHWM+1 to min idn)
CS->>CC: Batch of messages
CC->>PR: Fill in the gap
PR->>PR: Update CHWM
PR->>PR: Restart processor
else CHWM == idn-1
PR->>PR: Update CHWM
PR->>BF: Process event
end
Updated class diagram for DatabaseSetupclassDiagram
class DatabaseSetup {
-String jdbcUrl
-String username
-String password
+DatabaseSetup(String jdbcUrl, String username, String password)
+createSchemaIfNotExists() DatabaseSetup
+createTableIfNotExists(String topic) DatabaseSetup
+createMessagesTableIfNotExists() DatabaseSetup
-getConnection() Connection
}
Class diagram for CatchupServerclassDiagram
class CatchupServer {
-String topic
-DataSource dataSource
+CatchupServer(String topic, DataSource dataSource)
+fetchEvents(long start, long end, int maxResults) List<Event>
}
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
DatabaseSetupto drop the schema for testing purposes. - The
createSchemaIfNotExistsandcreateTableIfNotExistsmethods inDatabaseSetupcould be combined into a single method for better readability.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 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.
Comment on lines
+48
to
+49
| @Test | ||
| public void testFetchEvents() throws Exception { |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test case for fetching zero events.
It's important to test the scenario where no events are available within the specified range. This ensures the fetchEvents method handles this case gracefully and returns an empty list.
Suggested implementation:
}
@Test
public void testFetchZeroEvents() throws Exception {
// Do not publish any events. Verify fetchEvents returns an empty list.
List<Event> events = catchupServer.fetchEvents();
assertTrue(events.isEmpty(), "Expected no events to be fetched when none have been published");
}Make sure that the fetchEvents method is accessible and does not require any additional parameters for this test.
If fetchEvents requires parameters (for example, a time range or offset), modify the test accordingly and pass the appropriate values.
| * The catchup mechanism looks for the next gap (CHWM+1 upwards) and repeats until there are no gaps | ||
| * The catchup mechanism restarts the processor | ||
| * Request a batch of messages from the server | ||
| * Write each message to the consumer |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Inaccurate description of catchup mechanism
The catchup mechanism writes messages to the database, not directly to the consumer. The consumer then reads from the database.
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.
Addresses #22
Summary by Sourcery
Implements a catchup mechanism for persistent subscribers to handle gaps in the event sequence. This includes creating a CatchupServer to fetch missing events and updating the database setup to include a messages table.
New Features:
Tests: