-
Notifications
You must be signed in to change notification settings - Fork 3
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
backport logical backups #536
Conversation
Signed-off-by: 'Renan Rangel' <[email protected]>
Signed-off-by: 'Renan Rangel' <[email protected]>
…ssio#17000) Signed-off-by: Renan Rangel <[email protected]> Signed-off-by: 'Renan Rangel' <[email protected]>
Signed-off-by: 'Renan Rangel' <[email protected]>
Signed-off-by: 'Renan Rangel' <[email protected]>
@@ -352,21 +354,6 @@ func Restore(ctx context.Context, params RestoreParams) (*BackupManifest, error) | |||
return nil, err | |||
} |
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 like ShouldStartMySQLAfterRestore
is not checked online 350-352? or it's no longer needed? the original change https://github.com/vitessio/vitess/pull/16295/files#diff-b99512e7a77c7bd6ad4dcdd037ebc57754be363e0148c67bb22886cd4fbc1d80R450
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.
that's a good catch, thanks Tanjin. I think this got removed because we don't exactly need it on our setup because of the hooks, but there is no reason it shouldn't be backported too. I have added it there
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.
other than the comment about "ShouldStartMySQLAfterRestore", others LGTM by comparing to the upstream PR. 🎉
Signed-off-by: 'Renan Rangel' <[email protected]>
Description
This backports the new logical backups engine added upstream in vitessio#16295, as well as a fix for it in vitessio#17000
Related Issue(s)
Checklist
Deployment Notes