Introduce support for liveness check for MSSQL CDC listener#1169
Closed
gayaldassanayake wants to merge 4 commits intoballerina-platform:mainfrom
Closed
Introduce support for liveness check for MSSQL CDC listener#1169gayaldassanayake wants to merge 4 commits intoballerina-platform:mainfrom
gayaldassanayake wants to merge 4 commits intoballerina-platform:mainfrom
Conversation
gayaldassanayake
requested review from
aashikam,
niveathika and
shafreenAnfar
as code owners
📝 WalkthroughWalkthroughBumped mssql from 1.16.2 → 1.16.3 and updated compiler plugin/JAR paths; expanded dependency graph by adding avro, confluent.cavroserdes, confluent.cregistry, kafka and upgrading multiple stdlib versions; relaxed several package scopes; changed internal CdcListener config typing and population; added liveness tests and Gradle dependency entries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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: 1
🧹 Nitpick comments (1)
ballerina/Ballerina.toml (1)
15-19: Version declaration vs. SNAPSHOT path mismatch.The declared version is
1.16.3but the path referencesmssql-native-1.16.3-SNAPSHOT.jar. While this is a common pattern during development, ensure the path is updated to the release artifact (removing-SNAPSHOT) before merging to main or creating a release.🔧 Path to update before release
[[platform.java21.dependency]] groupId = "io.ballerina.stdlib" artifactId = "mssql-native" version = "1.16.3" -path = "../native/build/libs/mssql-native-1.16.3-SNAPSHOT.jar" +path = "../native/build/libs/mssql-native-1.16.3.jar"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ballerina/Ballerina.toml` around lines 15 - 19, The dependency block for [[platform.java21.dependency]] declares version "1.16.3" but the local path points to "mssql-native-1.16.3-SNAPSHOT.jar"; update the path in Ballerina.toml to the release artifact (remove "-SNAPSHOT") so it matches the declared version (e.g., change path to .../mssql-native-1.16.3.jar) before merging or releasing.
🤖 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/Ballerina.toml`:
- Line 4: The PR title references "MySQL CDC listener" but the change is in the
MSSQL (Microsoft SQL Server) module (see version = "1.16.3" in Ballerina.toml
and module context), so verify and correct the PR title and any related metadata
to reflect MSSQL if this is not a MySQL change; if the change truly targets
MySQL, update the module files to match or adjust the bumped file to the correct
module. Also check and update any changelog/commit messages and PR description
to consistently say "MSSQL CDC listener" (or the correct DB) so the title,
description, and files align.
---
Nitpick comments:
In `@ballerina/Ballerina.toml`:
- Around line 15-19: The dependency block for [[platform.java21.dependency]]
declares version "1.16.3" but the local path points to
"mssql-native-1.16.3-SNAPSHOT.jar"; update the path in Ballerina.toml to the
release artifact (remove "-SNAPSHOT") so it matches the declared version (e.g.,
change path to .../mssql-native-1.16.3.jar) before merging or releasing.
| org = "ballerinax" | ||
| name = "mssql" | ||
| version = "1.16.2" | ||
| version = "1.16.3" |
There was a problem hiding this comment.
Version bump looks good, but verify PR title accuracy.
The version bump to 1.16.3 is consistent across all configuration files. However, the PR title mentions "MySQL CDC listener" while this is the MSSQL (Microsoft SQL Server) module. Please verify if the PR title is accurate or if it's a copy-paste error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ballerina/Ballerina.toml` at line 4, The PR title references "MySQL CDC
listener" but the change is in the MSSQL (Microsoft SQL Server) module (see
version = "1.16.3" in Ballerina.toml and module context), so verify and correct
the PR title and any related metadata to reflect MSSQL if this is not a MySQL
change; if the change truly targets MySQL, update the module files to match or
adjust the bumped file to the correct module. Also check and update any
changelog/commit messages and PR description to consistently say "MSSQL CDC
listener" (or the correct DB) so the title, description, and files align.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ballerina/tests/listener_liveness_test.bal (2)
88-107: Test may be flaky depending on CDC event timing.This test relies on
runtime:sleep(10)being sufficient for the liveness interval (5.0 seconds) to expire without receiving events. If the test database generates CDC events (e.g., from other tests or background processes), this test could intermittently fail. Consider:
- Using a dedicated isolated database/table for this test
- Adding a test dependency to ensure it runs after other CDC tests complete
- Documenting the environmental requirements for this test
🤖 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, The test testLivenessWithoutReceivingEvents is flaky because runtime:sleep(10) may not guarantee no CDC events arrive within livenessInterval (5.0); fix by isolating the test's CDC input and adding a deterministic setup/teardown: create and use a dedicated temporary database/table (or unique cdcDatabase) before constructing the CdcListener (refer to CdcListener, livenessInterval, mssqlListener) and ensure the table is empty/disabled for writers, or add a test dependency/ordering to run after other CDC tests; alternatively increase the sleep to livenessInterval + safety margin and/or explicitly purge/verify no pending CDC events before calling cdc:isLive to make the assertion stable.
24-39: Consider adding cleanup for consistency.Unlike other test functions that call
gracefulStop(), this test doesn't clean up the listener. While the listener wasn't started, for consistency and to avoid potential resource leaks (e.g., ifattachallocates resources), consider adding a cleanup step.🔧 Suggested fix
check mssqlListener.attach(testService); boolean liveness = check cdc:isLive(mssqlListener); test:assertFalse(liveness, "Liveness check passes even before listener starts"); + check mssqlListener.gracefulStop(); }🤖 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 testLivenessBeforeListenerStart creates a CdcListener instance (mssqlListener) and calls attach but never performs cleanup; add a cleanup step that calls the listener's gracefulStop (e.g., invoke gracefulStop on mssqlListener) after the liveness assertion so resources from attach are released—ensure the call is awaited/checked (use check or handle the returned error) to match other tests' cleanup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gradle.properties`:
- Line 33: The gradle property stdlibCdcVersion is set to a timestamped snapshot
value (`1.2.0-20260213-180200-2268a4d`); change the value of stdlibCdcVersion to
the corresponding stable release (e.g., `1.2.0`) in gradle.properties before
merging so the build uses a released artifact instead of a timestamped/dev
snapshot.
---
Nitpick comments:
In `@ballerina/tests/listener_liveness_test.bal`:
- Around line 88-107: The test testLivenessWithoutReceivingEvents is flaky
because runtime:sleep(10) may not guarantee no CDC events arrive within
livenessInterval (5.0); fix by isolating the test's CDC input and adding a
deterministic setup/teardown: create and use a dedicated temporary
database/table (or unique cdcDatabase) before constructing the CdcListener
(refer to CdcListener, livenessInterval, mssqlListener) and ensure the table is
empty/disabled for writers, or add a test dependency/ordering to run after other
CDC tests; alternatively increase the sleep to livenessInterval + safety margin
and/or explicitly purge/verify no pending CDC events before calling cdc:isLive
to make the assertion stable.
- Around line 24-39: The test testLivenessBeforeListenerStart creates a
CdcListener instance (mssqlListener) and calls attach but never performs
cleanup; add a cleanup step that calls the listener's gracefulStop (e.g., invoke
gracefulStop on mssqlListener) after the liveness assertion so resources from
attach are released—ensure the call is awaited/checked (use check or handle the
returned error) to match other tests' cleanup behavior.
| observeInternalVersion=1.5.0 | ||
|
|
||
| stdlibCdcVersion=1.0.2 | ||
| stdlibCdcVersion=1.2.0-20260213-180200-2268a4d |
There was a problem hiding this comment.
Snapshot/timestamped version detected.
stdlibCdcVersion=1.2.0-20260213-180200-2268a4d appears to be a timestamped development build rather than a stable release version. This is acceptable during development but should be updated to a stable release version (e.g., 1.2.0) before merging to main.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gradle.properties` at line 33, The gradle property stdlibCdcVersion is set to
a timestamped snapshot value (`1.2.0-20260213-180200-2268a4d`); change the value
of stdlibCdcVersion to the corresponding stable release (e.g., `1.2.0`) in
gradle.properties before merging so the build uses a released artifact instead
of a timestamped/dev snapshot.
Contributor
Author
|
Closing because duplicate |
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.
Purpose
$ subject
Examples
Checklist
Summary
This pull request adds liveness check support to the MSSQL CDC listener and updates related dependencies and configuration so the listener can surface and use a liveness interval.
Changes
Implementation
Tests
Dependency and build updates
Configuration/manifest updates
Notes / Checklist