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
feat(bigtable): allow restore backup to different instance #3489
feat(bigtable): allow restore backup to different instance #3489
Conversation
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 fine to me aside from naming and docs issues. @AlisskaPie if we don't hear from you we'll just merge this into the branch as-is and make a separate PR to fix these issues before merging to master.
// | ||
// diffInstance is an instance in which the new table will be restored to. | ||
// Instance must be in the same project as the project containing backup. | ||
func (ac *AdminClient) RestoreTableTo(ctx context.Context, diffInstance, table, cluster, backup string) error { |
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.
instead of diffInstance I would do something like dstInstance. It also might be worth indicating that the other parameters are for the source. @kolea2 you can advise on the best names here.
|
||
// RestoreTableTo creates a new table by restoring from this completed backup. |
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.
I would try to make the doc here as parallel as possible to RestoreTable above, but clarify what is different. As it is I'm a bit confused about "completed" and whether that is also required for RestoreTable (but I'm not that familiar with how bigtable backups work.
@@ -2033,6 +2033,26 @@ func TestIntegration_AdminBackup(t *testing.T) { | |||
table := testEnv.Config().Table | |||
cluster := testEnv.Config().Cluster | |||
|
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.
Are there unit tests that cover RestoreTable currently or only integration tests? I don't see any unit tests that call RestoreTable directly but just want to make sure I'm not missing something. @kolea2
Merging as WIP to |
…4014) * feat(bigtable): allow restore backup to different instance (#3489) * feat: restore table to a different instance * add check that table restored * review fixes * change instance config zone * review feedback * cleanup docs and variable names * review feedback on test * review feedback * review feedback * review comments * rename variables * switch error * docs update Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com> * docs update Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com> Co-authored-by: Aleksandra Bogoslavetc <alexasha470@gmail.com> Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com>
No description provided.