Skip to content
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

Introducing migrationProjectId and decoupling Spanner projectId #802

Merged
merged 8 commits into from
Apr 5, 2024

Conversation

darshan-sj
Copy link
Collaborator

@darshan-sj darshan-sj commented Mar 27, 2024

Introduced migration project id. Utilized the new migration project id to create migration resources instead of spanner's project id.

Tested:

  • CLI
    • schema, data, schema-and-data, cleanup commands for Sharded Live migration
    • schema, data, schema-and-data, cleanup commands for Non Sharded Live migration
    • Both the above cases with different migration project id and spanner project id
  • UI
    • schema, data, schema-and-data, cleanup commands for Sharded Live migration
    • schema, data, schema-and-data, cleanup commands for Non Sharded Live migration

Pending tasks:

@darshan-sj darshan-sj requested a review from a team as a code owner March 27, 2024 09:44
@darshan-sj darshan-sj requested review from asthamohta and aksharauke and removed request for a team March 27, 2024 09:44
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Merging #802 (f52849c) into master (c5e5b2e) will decrease coverage by 0.04%.
The diff coverage is 21.01%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #802      +/-   ##
==========================================
- Coverage   45.96%   45.92%   -0.04%     
==========================================
  Files         166      166              
  Lines       20013    20032      +19     
  Branches      471      471              
==========================================
+ Hits         9199     9200       +1     
- Misses      10300    10318      +18     
  Partials      514      514              
Components Coverage Δ
backend-apis 44.38% <23.07%> (-0.09%) ⬇️
backend-library 50.56% <20.61%> (-0.04%) ⬇️
cli 0.00% <ø> (ø)
frontend 32.08% <ø> (ø)
Files Coverage Δ
common/utils/utils.go 0.00% <ø> (ø)
conversion/conversion.go 29.62% <100.00%> (ø)
conversion/mocks.go 91.66% <100.00%> (ø)
conversion/resource_generation.go 94.98% <100.00%> (ø)
sources/common/infoschema.go 0.00% <ø> (ø)
webv2/api/schema.go 57.86% <100.00%> (ø)
sources/dynamodb/schema.go 76.34% <0.00%> (ø)
sources/spanner/infoschema.go 5.72% <0.00%> (ø)
sources/sqlserver/infoschema.go 56.96% <0.00%> (ø)
webv2/session/session_handler.go 1.68% <0.00%> (ø)
... and 10 more

Copy link
Member

@manitgupta manitgupta left a comment

Choose a reason for hiding this comment

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

Overall looks good. Added some comments to check if there is an opportunity to simplify by using context instead of adding a new parameter wherever possible.

This is not a small amount to work to figure out quickly! Thanks a lot!

cmd/data.go Outdated Show resolved Hide resolved
cmd/data.go Show resolved Hide resolved
cmd/utils.go Outdated Show resolved Hide resolved
conversion/conversion.go Outdated Show resolved Hide resolved
conversion/conversion.go Show resolved Hide resolved
common/utils/utils.go Outdated Show resolved Hide resolved
cmd/data.go Show resolved Hide resolved
webv2/web.go Outdated Show resolved Hide resolved
Copy link

@krishnamoorthy-r krishnamoorthy-r left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, looks much bigger than what I originally thought!

cmd/data.go Outdated Show resolved Hide resolved
cmd/data.go Show resolved Hide resolved
conversion/conversion_from_source.go Show resolved Hide resolved
conversion/data_from_database.go Show resolved Hide resolved
Copy link
Member

@manitgupta manitgupta left a comment

Choose a reason for hiding this comment

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

Please update the PR description with -

  1. What is to be done as follow up work. Create bugs and linked them here.
  2. What testing has been done. I would recommend smoke testing the common pathways.

Copy link
Member

@manitgupta manitgupta left a comment

Choose a reason for hiding this comment

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

  1. Please also update the Github documentation with these changes
  2. Please update the PR description with what features break under what scenarios. For example - cleanup CLI will not work for cases when spanner project is different from migration resources project (and so on and so forth)

cmd/data.go Outdated Show resolved Hide resolved
@darshan-sj
Copy link
Collaborator Author

Updated the description with manual testing done and listed down the pending tasks.

@darshan-sj
Copy link
Collaborator Author

I have updated the github documentation as well

@darshan-sj darshan-sj merged commit 7681dce into master Apr 5, 2024
13 checks passed
@darshan-sj darshan-sj deleted the migrationProjectId-impl branch April 5, 2024 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants