-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Support shadow table in different database #2138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
...eam-to-spanner/src/main/java/com/google/cloud/teleport/v2/templates/DataStreamToSpanner.java
Outdated
Show resolved
Hide resolved
bharadwaj-aditya
left a comment
There was a problem hiding this 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.
...eam-to-spanner/src/main/java/com/google/cloud/teleport/v2/templates/DataStreamToSpanner.java
Outdated
Show resolved
Hide resolved
...anner/src/main/java/com/google/cloud/teleport/v2/templates/SpannerTransactionWriterDoFn.java
Outdated
Show resolved
Hide resolved
...anner/src/main/java/com/google/cloud/teleport/v2/templates/SpannerTransactionWriterDoFn.java
Show resolved
Hide resolved
...r/src/main/java/com/google/cloud/teleport/v2/templates/spanner/ProcessInformationSchema.java
Show resolved
Hide resolved
darshan-sj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Show resolved
Hide resolved
...eam-to-spanner/src/main/java/com/google/cloud/teleport/v2/templates/DataStreamToSpanner.java
Outdated
Show resolved
Hide resolved
...anner/src/main/java/com/google/cloud/teleport/v2/templates/SpannerTransactionWriterDoFn.java
Show resolved
Hide resolved
...anner/src/main/java/com/google/cloud/teleport/v2/templates/SpannerTransactionWriterDoFn.java
Show resolved
Hide resolved
...er/src/main/java/com/google/cloud/teleport/v2/templates/datastream/ShadowTableReadUtils.java
Show resolved
Hide resolved
Filed b/391169228 |
...r/src/main/java/com/google/cloud/teleport/v2/templates/spanner/ProcessInformationSchema.java
Show resolved
Hide resolved
...eam-to-spanner/src/main/java/com/google/cloud/teleport/v2/templates/DataStreamToSpanner.java
Show resolved
Hide resolved
bharadwaj-aditya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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:
shadowTableSpannerInstanceIdandshadowTableSpanneDatabaseId: 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.Manual Tests done:
Pending: