Repository navigation
test(Spanner): add SQL Server comprehensive integration tests for Datastream to Spanner pipeline - #4230
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #4230 +/- ##
=============================================
+ Coverage 40.97% 56.86% +15.88%
- Complexity 741 8085 +7344
=============================================
Files 210 1164 +954
Lines 15359 74117 +58758
Branches 1628 8735 +7107
=============================================
+ Hits 6294 42144 +35850
- Misses 8545 29170 +20625
- Partials 520 2803 +2283
🚀 New features to boost your workflow:
|
pratickchokhani
changed the title
b63ef92 to
44c9998
Compare
44c9998 to
c7efd4a
Compare
…o Spanner pipeline and update timestamp conversion logic
…o Spanner pipeline
- Rebase on main after GoogleCloudPlatform#4244 (static SQL Server container, shared datatype IT). - Align existing SQL Server ITs with the merged approach (enableCdcOnAllTables, shared SQLServerITContainer, resilient teardown); base class unchanged from main. - Add live-Datastream ports of the manifest scenarios: Events, DDL (custom transformation, size conversion, identity), Timezone, Session (rename/drop, synthetic PK), SeparateShadowTable PK-focused, SingleDF sharded (@ignore until migration_shard_id is populated for sqlserver sources). - Drop ITs not listed in manifest.yaml (RetryAllDLQ, sharded RetryDLQ, TableAndIndexLimits) and the onboarding report.
c7efd4a to
7c5cd31
Compare
- EventsIT: quote reserved keyword [plan] in SQL Server DDL/DML. - SQLServerDataStreamToSpannerIT: drop manual sp_cdc_* statements from the schema script; enableCdcOnAllTables() owns CDC setup. - RetryDLQIT: AllDataTypes.datetime_col -> DATETIMEOFFSET (datetime is routed to the DLQ), which left a second permanent event in the severe bucket. - SeparateShadowTablePKFocusedIT: render the expected string via Value.string(..).toString(), which elides values longer than 36 chars.
With several ITs enabling CDC concurrently on the shared SQL Server container, the first sp_cdc_enable_table per database deadlocks in msdb.dbo.sp_add_job (error 1205) and SQL Server asks to rerun the transaction. - CloudSqlServerResourceManager.enableCdcOnAllTables(): retry on deadlock with backoff; the cursor only visits untracked tables so reruns resume safely. - Add enableCdcOnTable(schema, table, uniqueIndexName) with the same retry for tables without a primary key. - SessionIT: enable CDC on the PK-less Books table through the resource manager instead of a raw EXEC in the DDL script.
Hold a static lock while enabling CDC so concurrent ITs on the shared SQL Server container never race on msdb job creation; keep the deadlock retry as a fallback for contention from outside the JVM.
enableCdcOnAllTables() now uses a table's first unique index as @index_name when it has no primary key, so the separate enableCdcOnTable helper and the capture-job helper are folded away.
Only the lock is needed for the deadlock; the PK-less Books table in the Session IT gets a primary key on the source side instead.
… row count The CDC wave's row count is also satisfied by transient states before the UPDATE of id 3 is applied, which made the final assertion racy.
- DDLIT / SessionIT: wait for the updated rows, not just the row count, after the CDC wave (same race as EventsIT). - RetryDLQIT: restore the 'wait for fixed rows' step from the MySQL reference before the final assertions. - Pass only the generated session content in SQLServerDataStreamToSpannerIT. - Delete Datastream streams before dropping the source database in the three ITs that cleaned up in the other order.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces critical stability improvements for the SQL Server resource manager by serializing CDC enablement to avoid deadlocks. Additionally, it significantly expands the test coverage for the Datastream to Spanner pipeline by adding a suite of integration tests that validate complex migration scenarios, including schema evolution, reserved keyword handling, and sharded data replication. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of integration tests for SQL Server to Spanner migrations using Datastream, covering various scenarios such as retry DLQ, reserved keywords, DDL conversions, events, sessions, timezones, sharded migrations, and separate shadow table databases. Additionally, it resolves a potential concurrent deadlock issue (SQL Server error 1205) in CloudSqlServerResourceManager by introducing a static lock (CDC_ENABLE_LOCK) to serialize CDC enablement. The reviewer feedback highlights a critical improvement opportunity across all the newly added integration tests: wrapping the Datastream resource manager cleanup in a try-finally block to guarantee that expensive cloud resources (like Spanner and Cloud SQL instances) are always cleaned up even if the Datastream cleanup fails.
…stead Each CDC wait now describes the exact end state of the wave (updated rows visible, deleted rows gone, inserted rows present), so the fixed cutover sleep before assertions is no longer needed.
cb0ec64 to
82b11f0
Compare
Datastream resource manager is passed first so streams are still deleted before the CDC-enabled source database is dropped.
860b41a to
c83c174
Compare
dhwanilpatel
left a comment
dhwanilpatel
left a comment
There was a problem hiding this comment.
Let's trigger ITs couple of time to validate multiple ITs are working well against static testcontainer.
| @Category({TemplateIntegrationTest.class, SkipDirectRunnerTest.class}) | ||
| @TemplateIntegrationTest(DataStreamToSpanner.class) | ||
| @RunWith(JUnit4.class) | ||
| public class SQLServerDataStreamToSpannerDDLIT extends DataStreamToSpannerITBase { |
There was a problem hiding this comment.
I think DDL IT has more ITs, can you double check with mysql DDL IT if some tests are missing from it?
There was a problem hiding this comment.
I omitted those because they were covered by other ITs. Have added them.
| // Insert initial data | ||
| insertSqlServerData("source_table1", 1, "Alice", 30, "Y", "2026-01-01"); | ||
| insertSqlServerData("source_table2", 1, "Bob", 40, "N", "2026-01-02"); |
There was a problem hiding this comment.
It seems this is only testing inserts, we will need to test updates/delete as well. Reference mysql test verifies for all operations. Let's update this accordingly.
| @After | ||
| public void cleanUp() { | ||
| LOG.info("Cleaning up resources..."); | ||
| // Datastream streams are deleted first, before the CDC-enabled source database is dropped. | ||
| ResourceManagerUtils.cleanResources( | ||
| datastreamResourceManager, | ||
| msSqlResourceManager, | ||
| spannerResourceManager, | ||
| gcsResourceManager, | ||
| pubsubResourceManager); | ||
| } |
There was a problem hiding this comment.
From gemini : Let's check for other class as well.
Subclass @after teardown deletes live GCP/DB resources while the streaming Dataflow job is still running
- Locations:
- DatastreamToSpannerReservedKeywordsSqlServerIT.java:124-134
- SQLServerDataStreamToSpannerIT.java:113-122
- SQLServerDataStreamToSpannerTimezoneIT.java:124-134
- SQLServerSeparateShadowTableDatabasePKFocusedIT.java:134-145
- Issue: In JUnit 4, subclass @after methods run before superclass @after methods (TemplateTestBase.baseCleanup()). Because these classes don't set
skipBaseCleanup = true and call tearDownBase() first, cleanUp() deletes the Datastream stream, SQL Server DB, Spanner DB, GCS objects, and Pub/Sub subscriptions
before tearDownBase() cancels the running Dataflow job (and can invoke gcsClient.cleanupAll() twice). - Fix: Set skipBaseCleanup = true in @before and call tearDownBase() at the start of cleanUp() before cleaning up resource managers (matching
OracleDataStreamToSpannerIT.java:118-145).
| * | ||
| * @throws IOException | ||
| */ | ||
| @AfterClass |
There was a problem hiding this comment.
This class has multiple methods which will spin up different dataflow jobs but those will be cleaned at the end of class itself. Let's clean those at the end of test itself.
There was a problem hiding this comment.
There is only 1 dataflow job for the whole class.
- Cancel the Dataflow job (tearDownBase) before deleting its resources in the per-test ITs (ReservedKeywords, SQLServerDataStreamToSpannerIT, Timezone). - SQLServerDataStreamToSpannerIT: verify UPDATE and DELETE in addition to INSERT, like the MySQL reference. - DDLIT: port DatatypeSizeReducedConversion (oversized values rejected, not truncated) and SequenceColumns (SQL Server SEQUENCE key default).
SQL Server pads CHAR(n) to n characters, so the 'fits' row never fit the reduced STRING(5) column either. Keep char_column at full width and apply the size reduction to the NVARCHAR column; drop the varbinary column from the table.
No description provided.