Point in time recovery and restore: assume (and validate) MySQL56 flavor in position arguments #15599
Conversation
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
…Retire (but not purge as it may still be useful) DecodePositionDefaultFlavor Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
shlomi-noach
added
Type: Internal Cleanup
Type: Enhancement
shlomi-noach
requested review from
deepthi,
GuptaManan100,
rohit-nayak-ps,
harshit-gangal,
systay and
frouioui
as code owners
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15599 +/- ##
==========================================
+ Coverage 65.78% 68.08% +2.30%
==========================================
Files 1561 1561
Lines 194838 195316 +478
==========================================
+ Hits 128171 132981 +4810
+ Misses 66667 62335 -4332 ☔ View full report in Codecov by Sentry. |
shlomi-noach
added
Component: Backup and Restore
and removed
Backport
| if err != nil { | ||
| return nil, vterrors.Wrapf(err, "cannot decode position in incremental backup: %v", incrementalFromPos) | ||
| } | ||
| if !pos.MatchesFlavor(replication.Mysql56FlavorID) { | ||
| return nil, vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "incremental backup only supports MySQL GTID positions. Got: %v", incrementalFromPos) |
There was a problem hiding this comment.
IMO it would be nice to move this wording to the error above so that it's clear that we don't support other flavors.
There was a problem hiding this comment.
This is likewise an intentional design: we Wrapf an error, which happens to be ErrExpectMysql56Flavor, and which explicitly tells us "expected MySQL GTID position but found a different or invalid format". It would be redundant to add the same wording here.
There was a problem hiding this comment.
Right, but we lose the wording which notes that MySQL format is the only one supported. That was my point. I guess it's implicit in this wording, so not a huge deal.
There was a problem hiding this comment.
We don't lose the wording. The error above will have both cannot decode position in incremental backup: %v as well as "expected MySQL GTID position but found a different or invalid format, because it wraps one with another. Am I misunderstanding?
| pos, gtidSet, err := DecodePositionMySQL56("MySQL56/16b1039f-22b6-11ed-b765-0a43f95f28a3:1-615") | ||
| assert.NoError(t, err) | ||
| assert.False(t, pos.IsZero()) | ||
| assert.NotNil(t, gtidSet) |
There was a problem hiding this comment.
Isn't it worth asserting that the provided gtidSet is not being mangled in any way, and makes it here intact?
| pos, gtidSet, err := DecodePositionMySQL56("16b1039f-22b6-11ed-b765-0a43f95f28a3:1-615") | ||
| assert.NoError(t, err) | ||
| assert.False(t, pos.IsZero()) | ||
| assert.NotNil(t, gtidSet) |
There was a problem hiding this comment.
Similarly here, it will be more robust to assert on the actual result versus only asserting that it is not nil.
There was a problem hiding this comment.
We do have a utils.MustMatch somewhere that can be used to check for equality of complex structs.
There was a problem hiding this comment.
Done. No need for utils.MustMatch, assert.Equal(...) implements deep check.
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
|
Looking for another review 🙏 |
Description
Incremental backup and point-in-time recoveries assume a MySQL 5.6 flavor GTID. To that effect, both now:
"MySQL56/"prefix in position argument"MySQL56/"prefix, the positiion must be in MySQL 5.6 GTID formatThis PR consolidated the logic into a new
DecodePositionMySQL56()function ingo/mysql/replication. We retireDecodePositionDefaultFlavor()though we still keep the function as it may yet prove useful.Added unit tests, and updated endtoend tests to use positions with/out
"MySQL56/"prefix.Related Issue(s)
Checklist
Deployment Notes