Support shadow table in different database by Deep1998 · Pull Request #2138 · GoogleCloudPlatform/DataflowTemplates

Deep1998 · 2025-01-17T12:03:09Z

This change introduces support for shadow table in a separate database. The code changes aim to minimise the existing live flow changes as much as possible.

Summary of changes:

Introduced 2 new flags shadowTableSpannerInstanceId and shadowTableSpanneDatabaseId: if both are provided we use a separate shadow table database, if neither are provided, we use the target database as the shadow table db as per the existing flow.
ProcessInformationSchema: Earlier this used to return a single ddl object containing both main and shadow tables in a single ddl object. It now separates main table and shadow tables into 2 separate ddl objects that are passed around in the codebase.
TransactionWriterDoFn now uses the cross db transaction only if shadow table is in a separate db.
Shadow Table Sequence reader reads with an exclusive lock in case of a cross db transaction. This requires usin gsql statements for querying. For normal flows, we use the read api as is.

Manual Tests done:

INSERT/UPDATE/DELETE on table with primary key consisting of all datatypes possible (JSON we are not supporting in the interim soln).
UPDATE primary key itself which generates DELETE and INSERT event by Datastream which is also handled.
Unspecified shadowtable instance and db params, defaults to main db for shadow table.

Pending:

Integration Tests: these will be taken in a follow up PR

codecov · 2025-01-17T12:22:54Z

Codecov Report

Attention: Patch coverage is 59.30736% with 94 lines in your changes missing coverage. Please review.

Project coverage is 54.86%. Comparing base (f933ae9) to head (949fca3).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...ort/v2/templates/SpannerTransactionWriterDoFn.java	27.94%	45 Missing and 4 partials ⚠️
...v2/templates/spanner/ProcessInformationSchema.java	53.19%	22 Missing ⚠️
...oud/teleport/v2/templates/DataStreamToSpanner.java	76.31%	8 Missing and 1 partial ⚠️
...eleport/v2/templates/SpannerTransactionWriter.java	0.00%	3 Missing ⚠️
.../v2/templates/datastream/ShadowTableReadUtils.java	91.66%	2 Missing and 1 partial ⚠️
...rt/v2/templates/datastream/ChangeEventContext.java	33.33%	2 Missing ⚠️
...templates/datastream/MySqlChangeEventSequence.java	77.77%	1 Missing and 1 partial ⚠️
...emplates/datastream/OracleChangeEventSequence.java	77.77%	1 Missing and 1 partial ⚠️
...plates/datastream/PostgresChangeEventSequence.java	77.77%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2138      +/-   ##
============================================
+ Coverage     46.62%   54.86%   +8.24%     
+ Complexity     3953     1595    -2358     
============================================
  Files           867      406     -461     
  Lines         51567    22015   -29552     
  Branches       5399     2184    -3215     
============================================
- Hits          24043    12079   -11964     
+ Misses        25808     9231   -16577     
+ Partials       1716      705    -1011

Components	Coverage Δ
spanner-templates	`70.07% <59.30%> (+1.44%)`	⬆️
spanner-import-export	`∅ <ø> (∅)`
spanner-live-forward-migration	`76.49% <59.30%> (-1.00%)`	⬇️
spanner-live-reverse-replication	`78.39% <ø> (-0.05%)`	⬇️
spanner-bulk-migration	`87.87% <ø> (+0.20%)`	⬆️

Files with missing lines	Coverage Δ
...emplates/datastream/ChangeEventContextFactory.java	`100.00% <100.00%> (ø)`
...mplates/datastream/ChangeEventSequenceFactory.java	`76.00% <100.00%> (ø)`
.../templates/datastream/MySqlChangeEventContext.java	`100.00% <100.00%> (ø)`
...templates/datastream/OracleChangeEventContext.java	`100.00% <100.00%> (ø)`
...mplates/datastream/PostgresChangeEventContext.java	`100.00% <100.00%> (ø)`
...rt/v2/templates/datastream/ChangeEventContext.java	`88.23% <33.33%> (-11.77%)`	⬇️
...templates/datastream/MySqlChangeEventSequence.java	`86.79% <77.77%> (-2.10%)`	⬇️
...emplates/datastream/OracleChangeEventSequence.java	`82.92% <77.77%> (-1.93%)`	⬇️
...plates/datastream/PostgresChangeEventSequence.java	`86.00% <77.77%> (-2.10%)`	⬇️
...eleport/v2/templates/SpannerTransactionWriter.java	`0.00% <0.00%> (ø)`
... and 4 more

... and 492 files with indirect coverage changes

...eam-to-spanner/src/main/java/com/google/cloud/teleport/v2/templates/DataStreamToSpanner.java

bharadwaj-aditya

Apart from the comments, can you check for places to bump up the test coverage. Overall on this PR it seems low.

...eam-to-spanner/src/main/java/com/google/cloud/teleport/v2/templates/DataStreamToSpanner.java

...anner/src/main/java/com/google/cloud/teleport/v2/templates/SpannerTransactionWriterDoFn.java

...r/src/main/java/com/google/cloud/teleport/v2/templates/spanner/ProcessInformationSchema.java

darshan-sj

Looks good overall!

Can you please create Bugs to update all the integration tests and Load tests?
Can you document manual testing which you did in the PR description.

...eam-to-spanner/src/main/java/com/google/cloud/teleport/v2/templates/DataStreamToSpanner.java

...anner/src/main/java/com/google/cloud/teleport/v2/templates/SpannerTransactionWriterDoFn.java

...er/src/main/java/com/google/cloud/teleport/v2/templates/datastream/ShadowTableReadUtils.java

Deep1998 · 2025-01-20T17:33:23Z

Looks good overall!

Can you please create Bugs to update all the integration tests and Load tests?

Can you document manual testing which you did in the PR description.

Filed b/391169228

...r/src/main/java/com/google/cloud/teleport/v2/templates/spanner/ProcessInformationSchema.java

...eam-to-spanner/src/main/java/com/google/cloud/teleport/v2/templates/DataStreamToSpanner.java

bharadwaj-aditya

LGTM

Deep1998 requested a review from a team as a code owner January 17, 2025 12:03

Deep1998 requested review from asthamohta and darshan-sj January 17, 2025 12:03

pull-request-size bot added the size/XXL label Jan 17, 2025

Changes with SQL Statements; Latest version in unit test

405cddf

Deep1998 force-pushed the ea-fix branch from e4f4171 to 405cddf Compare January 17, 2025 12:13

bharadwaj-aditya reviewed Jan 17, 2025

View reviewed changes

...eam-to-spanner/src/main/java/com/google/cloud/teleport/v2/templates/DataStreamToSpanner.java Outdated Show resolved Hide resolved

bharadwaj-aditya requested changes Jan 17, 2025

View reviewed changes

darshan-sj added the improvement Making existing code better label Jan 17, 2025

Resolve comments and add unit test coverage

949fca3

darshan-sj approved these changes Jan 20, 2025

View reviewed changes

bharadwaj-aditya reviewed Jan 21, 2025

View reviewed changes

...r/src/main/java/com/google/cloud/teleport/v2/templates/spanner/ProcessInformationSchema.java Show resolved Hide resolved

bharadwaj-aditya reviewed Jan 21, 2025

View reviewed changes

...eam-to-spanner/src/main/java/com/google/cloud/teleport/v2/templates/DataStreamToSpanner.java Show resolved Hide resolved

bharadwaj-aditya approved these changes Jan 21, 2025

View reviewed changes

Deep1998 merged commit a2779d4 into GoogleCloudPlatform:main Jan 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support shadow table in different database#2138

Support shadow table in different database#2138
Deep1998 merged 2 commits intoGoogleCloudPlatform:mainfrom
Deep1998:ea-fix

Deep1998 commented Jan 17, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jan 17, 2025 •

edited

Loading

Uh oh!

Uh oh!

bharadwaj-aditya left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

darshan-sj left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Deep1998 commented Jan 20, 2025

Uh oh!

Uh oh!

Uh oh!

bharadwaj-aditya left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Deep1998 commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

bharadwaj-aditya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

darshan-sj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Deep1998 commented Jan 20, 2025

Uh oh!

Uh oh!

Uh oh!

bharadwaj-aditya left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Deep1998 commented Jan 17, 2025 •

edited

Loading

codecov bot commented Jan 17, 2025 •

edited

Loading