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

backport logical backups #536

Merged
merged 7 commits into from
Oct 25, 2024
Merged

backport logical backups #536

merged 7 commits into from
Oct 25, 2024

Conversation

rvrangel
Copy link
Collaborator

@rvrangel rvrangel commented Oct 17, 2024

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

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on the CI
  • Documentation was added or is not required

Deployment Notes

Signed-off-by: 'Renan Rangel' <[email protected]>
@github-actions github-actions bot added this to the v15.0.5 milestone Oct 17, 2024
@rvrangel rvrangel marked this pull request as ready for review October 24, 2024 13:38
@rvrangel rvrangel requested a review from a team as a code owner October 24, 2024 13:38
@@ -352,21 +354,6 @@ func Restore(ctx context.Context, params RestoreParams) (*BackupManifest, error)
return nil, err
}
Copy link

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

Copy link
Collaborator Author

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

tanjinx
tanjinx previously approved these changes Oct 24, 2024
Copy link

@tanjinx tanjinx left a 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. 🎉

@rvrangel rvrangel merged commit 1ba76cd into slack-15.0 Oct 25, 2024
194 checks passed
@rvrangel rvrangel deleted the logical-backups-backport2 branch October 25, 2024 14:21
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.

3 participants