Skip to content

Conversation

@Deep1998
Copy link
Contributor

@Deep1998 Deep1998 commented Jan 17, 2025

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:

  1. 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.
  2. 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.
  3. TransactionWriterDoFn now uses the cross db transaction only if shadow table is in a separate db.
  4. 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

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

codecov bot commented Jan 17, 2025

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

Copy link
Contributor

@bharadwaj-aditya bharadwaj-aditya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@darshan-sj darshan-sj added the improvement Making existing code better label Jan 17, 2025
Copy link
Contributor

@darshan-sj darshan-sj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall!

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

@Deep1998
Copy link
Contributor Author

Looks good overall!

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

Filed b/391169228

Copy link
Contributor

@bharadwaj-aditya bharadwaj-aditya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Deep1998 Deep1998 merged commit a2779d4 into GoogleCloudPlatform:main Jan 21, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Making existing code better size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants