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

Backup using vtbackup endtoend test cases in Go migrated from Python #5693

Merged
merged 6 commits into from
Jan 29, 2020

Conversation

ajeetj
Copy link
Contributor

@ajeetj ajeetj commented Jan 13, 2020

Migrated vtbackup testcases from python to go.

@ajeetj ajeetj requested a review from sougou as a code owner January 13, 2020 04:34
@ajeetj ajeetj changed the title [WIP] Backup using vtbackup endtoend test cases in Go migrated from Python Backup using vtbackup endtoend test cases in Go migrated from Python Jan 21, 2020
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

Mostly looks good.
It will be nice to reuse common code for list/count/remove backups.
Also, the merge conflict needs to be resolved.

assert.Nil(t, err)
}

func listBackups(t *testing.T) string {
Copy link
Member

Choose a reason for hiding this comment

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

can these local functions be replaced by shared code?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was the plan.
But Pradip also started making changes in that common code. And if I update it in my PR then there will be a lot of merge conflicts. We will make sure it is cleaned up in his PR.

Copy link
Member

Choose a reason for hiding this comment

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

ok I'll approve for now and you can fix this in the other PR.

Comment on lines +295 to +297
//Tear down Tablet
//err := tablet.VttabletProcess.TearDown()
//assert.Nil(t, err)
Copy link
Member

Choose a reason for hiding this comment

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

If this is no longer relevant, delete instead of commenting out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not 100% sure.
As we are restarting the mysql here the rest of the code is not at all required. Like promoteSlaveCommands & disableSemiSyncCommands

Ideally, we will have to uncomment the commented code and comment resetTabletDirectory(t, tablet, initMysql). But that doesn't work

Copy link
Member

Choose a reason for hiding this comment

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

Let's get this merged and investigate that separately.

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

LGTM

@deepthi deepthi merged commit a577182 into vitessio:master Jan 29, 2020
@deepthi deepthi deleted the tal_backup_only branch September 9, 2020 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants