Open Introduce support for liveness check for MSSQL CDC listener#1245
Open Introduce support for liveness check for MSSQL CDC listener#1245gayaldassanayake merged 4 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughBumps multiple package versions (including PostgreSQL from 1.16.2→1.16.3), adds Avro/Kafka/Confluent/UUID/log packages, changes CdcListener.config type from Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant CdcListener
participant Debezium
participant PostgresDB
Test->>CdcListener: create(listenerConfig with livenessInterval?)
CdcListener->>Debezium: build debeziumConfigs (includes DB + livenessInterval)
CdcListener->>Debezium: start listener
Debezium->>PostgresDB: subscribe / stream events
Note right of Debezium: events flow or no events
Test->>CdcListener: cdc:isLive()
CdcListener->>Test: return true/false based on livenessInterval & event activity
Test->>CdcListener: stop()
CdcListener->>Debezium: stop subscription
Debezium->>PostgresDB: unsubscribe
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ballerina/tests/listener_liveness_test.bal (1)
88-107: Consider adding a test for liveness recovery after receiving events.The current
testLivenessWithoutReceivingEventstest validates that liveness becomes false when no events are received within the interval. For completeness, consider adding a complementary test that verifies liveness returns to true when events are received again after a period of inactivity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ballerina/tests/listener_liveness_test.bal` around lines 88 - 107, Add a complementary test (e.g., testLivenessRecoveryAfterReceivingEvents) that mirrors testLivenessWithoutReceivingEvents but verifies liveness returns true after events resume: create a CdcListener with a short livenessInterval, attach it to testService and start it (use postgresqlListener.attach and postgresqlListener.'start()), wait until cdc:isLive(postgresqlListener) becomes false (use runtime:sleep and cdc:isLive), then simulate/send an event to the service so the listener receives data, wait a short period and assert cdc:isLive(postgresqlListener) is true, and finally call postgresqlListener.gracefulStop(); ensure the test references the same symbols (CdcListener, livenessInterval, testService, cdc:isLive, gracefulStop) so it follows the existing pattern.gradle.properties (1)
63-63: Snapshot dependency for CDC module may cause build reproducibility issues.The
stdlibCdcVersionuses a timestamped snapshot version (1.2.0-20260213-180200-2268a4d). Before merging, ensure this is updated to a stable release version to avoid:
- Non-reproducible builds
- Potential breakage if the snapshot artifact is removed from the repository
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle.properties` at line 63, The gradle property stdlibCdcVersion is set to a timestamped snapshot (1.2.0-20260213-180200-2268a4d) which harms reproducibility; update the stdlibCdcVersion property to a stable released version (replace the snapshot string with the corresponding released version tag, e.g., 1.2.0 or the exact stable release your project depends on) so builds no longer rely on ephemeral snapshot artifacts and ensure any consumers or dependency constraints (build scripts that reference stdlibCdcVersion) are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ballerina/tests/listener_liveness_test.bal`:
- Around line 24-39: The test attaches testService to postgresqlListener but
never detaches or stops it; update testLivenessBeforeListenerStart to always
clean up by detaching the service (and stopping the listener if applicable). Add
a deferred/cleanup block after creating postgresqlListener that calls
postgresqlListener.detach(testService) (and postgresqlListener.stop() if the
listener API exposes stop/close) and handle/propagate any errors consistently
(use check or swallow with ignore) so resources are released even on test
failures.
---
Nitpick comments:
In `@ballerina/tests/listener_liveness_test.bal`:
- Around line 88-107: Add a complementary test (e.g.,
testLivenessRecoveryAfterReceivingEvents) that mirrors
testLivenessWithoutReceivingEvents but verifies liveness returns true after
events resume: create a CdcListener with a short livenessInterval, attach it to
testService and start it (use postgresqlListener.attach and
postgresqlListener.'start()), wait until cdc:isLive(postgresqlListener) becomes
false (use runtime:sleep and cdc:isLive), then simulate/send an event to the
service so the listener receives data, wait a short period and assert
cdc:isLive(postgresqlListener) is true, and finally call
postgresqlListener.gracefulStop(); ensure the test references the same symbols
(CdcListener, livenessInterval, testService, cdc:isLive, gracefulStop) so it
follows the existing pattern.
In `@gradle.properties`:
- Line 63: The gradle property stdlibCdcVersion is set to a timestamped snapshot
(1.2.0-20260213-180200-2268a4d) which harms reproducibility; update the
stdlibCdcVersion property to a stable released version (replace the snapshot
string with the corresponding released version tag, e.g., 1.2.0 or the exact
stable release your project depends on) so builds no longer rely on ephemeral
snapshot artifacts and ensure any consumers or dependency constraints (build
scripts that reference stdlibCdcVersion) are updated accordingly.
| function testLivenessBeforeListenerStart() returns error? { | ||
| CdcListener postgresqlListener = new ({ | ||
| database: { | ||
| username: cdcUsername, | ||
| password: cdcPassword, | ||
| port: cdcPort, | ||
| databaseName: cdcDatabase | ||
| }, | ||
| options: { | ||
| snapshotMode: cdc:NO_DATA | ||
| } | ||
| }); | ||
| check postgresqlListener.attach(testService); | ||
| boolean liveness = check cdc:isLive(postgresqlListener); | ||
| test:assertFalse(liveness, "Liveness check passes even before listener starts"); | ||
| } |
There was a problem hiding this comment.
Missing listener cleanup in testLivenessBeforeListenerStart.
The test attaches a service to the listener but never detaches or stops it. While the listener was never started, it's good practice to clean up attached resources to prevent potential side effects on subsequent tests.
🧹 Proposed fix to add cleanup
check postgresqlListener.attach(testService);
boolean liveness = check cdc:isLive(postgresqlListener);
test:assertFalse(liveness, "Liveness check passes even before listener starts");
+ check postgresqlListener.detach(testService);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function testLivenessBeforeListenerStart() returns error? { | |
| CdcListener postgresqlListener = new ({ | |
| database: { | |
| username: cdcUsername, | |
| password: cdcPassword, | |
| port: cdcPort, | |
| databaseName: cdcDatabase | |
| }, | |
| options: { | |
| snapshotMode: cdc:NO_DATA | |
| } | |
| }); | |
| check postgresqlListener.attach(testService); | |
| boolean liveness = check cdc:isLive(postgresqlListener); | |
| test:assertFalse(liveness, "Liveness check passes even before listener starts"); | |
| } | |
| function testLivenessBeforeListenerStart() returns error? { | |
| CdcListener postgresqlListener = new ({ | |
| database: { | |
| username: cdcUsername, | |
| password: cdcPassword, | |
| port: cdcPort, | |
| databaseName: cdcDatabase | |
| }, | |
| options: { | |
| snapshotMode: cdc:NO_DATA | |
| } | |
| }); | |
| check postgresqlListener.attach(testService); | |
| boolean liveness = check cdc:isLive(postgresqlListener); | |
| test:assertFalse(liveness, "Liveness check passes even before listener starts"); | |
| check postgresqlListener.detach(testService); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ballerina/tests/listener_liveness_test.bal` around lines 24 - 39, The test
attaches testService to postgresqlListener but never detaches or stops it;
update testLivenessBeforeListenerStart to always clean up by detaching the
service (and stopping the listener if applicable). Add a deferred/cleanup block
after creating postgresqlListener that calls
postgresqlListener.detach(testService) (and postgresqlListener.stop() if the
listener API exposes stop/close) and handle/propagate any errors consistently
(use check or swallow with ignore) so resources are released even on test
failures.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1245 +/- ##
============================================
- Coverage 81.62% 81.57% -0.05%
+ Complexity 864 863 -1
============================================
Files 28 28
Lines 3521 3523 +2
Branches 484 484
============================================
Hits 2874 2874
- Misses 431 432 +1
- Partials 216 217 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Purpose
$ subject
Examples
Checklist
Summary
This pull request adds liveness-check support for the MSSQL CDC listener, enabling applications to determine whether a CDC listener instance is active and receiving events.
Key changes
Core functionality
Tests
Dependency and build updates
Impact
Provides standardized liveness observability for MSSQL CDC listeners, improving operational monitoring and allowing callers to detect inactive or stopped listeners programmatically. Existing public APIs remain unchanged aside from the config type relaxation described above.