-
Notifications
You must be signed in to change notification settings - Fork 12
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
Restrict live migration to single table-list and added guardrails for that around table-list flags #2354
base: main
Are you sure you want to change the base?
Conversation
…es on tables that are not part of migrations
…ctName and then re-using the code to first check if it is leaf table else lookup
…ags storing list with leaf in msr directly
850a85b
to
ce05787
Compare
bf3e4e0
to
f09e16f
Compare
… msr new fields with nil etc..
@@ -1040,6 +1040,9 @@ func storeTableListInMSR(tableList []sqlname.NameTuple) error { | |||
})) | |||
err := metaDB.UpdateMigrationStatusRecord(func(record *metadb.MigrationStatusRecord) { | |||
record.TableListExportedFromSource = minQuotedTableList |
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.
nit: minQuotedTableList
-> minQuotedTableListWithRoots
yb-voyager/cmd/exportData.go
Outdated
} | ||
|
||
if len(lo.Keys(newLeafTables)) > 0 { | ||
utils.PrintAndLog("Detected new partition tables for the following partitioned tables. These will not be considered during migration:") |
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.
Should we prompt?
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.
Not sure if a prompt is required, I think there is nothing we want users to do here right? so this might just FYI?
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.
Went over it at a high level, looks good!
Will give it a more detailed look soon along with the tests.
… DDL changes on source, subsequent run with table-list/exclude-table-list flags. Some TODOs remaining to added assertions for
yb-voyager/cmd/exportData_test.go
Outdated
|
||
// getInitialTableList for subsequent run with start-clean false and basic without table-list flags so no guardrails | ||
startClean = false | ||
getInitialTableistAndAssertExpectedResult(t, expectedTableList, expectedPartitionsToRootMap) |
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.
you added a new partition leaf above. This should error out no?
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.
We are not errorring out in this case just reporting to the user that new leafs found for these root tables and these will not be considered during migration,
yb-voyager/cmd/exportData_test.go
Outdated
|
||
getInitialTableistAndAssertExpectedResult(t, expectedTableList, expectedPartitionsToRootMap) | ||
|
||
//case2: getInitialTableList for subsequent run with start-clean false and basic with no table-list flags so no guardrails |
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.
If first run has table-list, and subsequent run does not have table-list, we should fail no?
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.
We don't fail; we use the stored one directly. Not sure if we should fail or not. Maybe we can have a prompt in this case as well saying using the initial table list ...
…ls and new leafs addition. Added tests for subsequent run validating the guardrails of missing and extra tables and new leaf tables addition
…ion as a function variable to be able to override it in the test
…s by adding a new field in nameRegfor Sequences detemination
3ed77c3
to
b034db2
Compare
@@ -552,12 +559,12 @@ func addLeafPartitionsInTableList(tableList []sqlname.NameTuple, ifTableListSet | |||
allLeafPartitions := GetAllLeafPartitions(table) | |||
prevLengthOfList := len(modifiedTableList) | |||
switch true { | |||
case len(allLeafPartitions) == 0 && rootTable != table: //leaf partition | |||
case len(allLeafPartitions) == 0 && !rootTable.Equals(table): //leaf partition |
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.
Does our NameTuple.Equals
work if the pointers are different, but the underlying values are the same?
Is that what prompted you to make this change?
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.
Yes, it does the DeepEqual
internally on the two nametuples.
yb-voyager/cmd/exportData.go
Outdated
@@ -576,7 +583,12 @@ func addLeafPartitionsInTableList(tableList []sqlname.NameTuple, ifTableListSet | |||
func GetRootTableOfPartition(table sqlname.NameTuple) (sqlname.NameTuple, error) { | |||
parentTable := source.DB().ParentTableOfPartition(table) | |||
if parentTable == "" { | |||
return table, nil | |||
//now we know its a root table so return the tuple from the nameregistry |
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.
can you give more context in the comment on why you need to lookup from nameregistry again?
For my clarification: this is because the original tuples do not have the target-side of names, right?
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.
Yes, right. The original tuples are hand-crafted ones without target names, so for the root, we shouldn't use these but instead get a proper one from the name registry.
var err error | ||
var includeTableList, excludeTableList []sqlname.NameTuple | ||
|
||
applyFilterAndAddLeafTable := func(flagList string, flagName string) ([]sqlname.NameTuple, 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.
This structure of applying include/exclude is relatively easy to understand, thanks 👍
yb-voyager/cmd/exportData.go
Outdated
} else { | ||
tableList = fullTableList | ||
//this is only for filtering the non-leaf and non-root tables - the mid level partitioned table from fullTableList |
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.
To remove non-leaf and non-root, you're still calling addLeafPartitionsInTableList
? That sounds a bit weird. Does that function ignore all leafs, and then re-add them?
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.
addLeafPartitionsInTableList
has a boolean parameter addAllLeafPartitions
in case this is false, that function will consider that the table-list passed is already exhaustive with all the tables (root, middle partition, leaf partitions, so on..) and it will just do the filtering to remove the middle level partitions and just the keep the leaf and root tables in the list.
yb-voyager/cmd/exportData.go
Outdated
partitionsToRootTableMap = msr.TargetRenameTablesMap | ||
|
||
//For the first run of export data from target we use the TableListExportedFromSource (which has only root tables) | ||
if len(msr.TargetExportedTableListWithLeafPartitions) == 0 || msr.TargetRenameTablesMap == nil { |
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.
nit: create a variable for this since this is used below as well.
yb-voyager/cmd/exportData.go
Outdated
}) | ||
|
||
// Finding all the partitions of all root tables part of migration, and report if there any new partitions added | ||
rootToNewLeafTablesMap, err := detectNewLeafPartitionsOnPartitionedTables(rootTables, registeredList) |
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.
should we move this block below to where we are actually checking rootToNewLeafTablesMap
and doing guardrails?
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.
we use the rootToNewLeafTablesMap to calculate the currentList
yb-voyager/cmd/exportData.go
Outdated
return nil, nil, fmt.Errorf("error applying table list flags for current list and remove roots: %v", err) | ||
} | ||
|
||
if len(lo.Keys(rootToNewLeafTablesMap)) > 0 { |
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.
shouldn't this check be done before the source.TableList == "" && source.ExcludeTableList == ""
if block?
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.
moved it in the detectNewLeafPartitionsOnPartitionedTable
function back.
yb-voyager/cmd/exportData.go
Outdated
|
||
} | ||
|
||
func applyTableListFlagsOnCurrentAndRemoveRootsFromBothLists(registeredList []sqlname.NameTuple, tableListViaFlag string, excludeTableListViaFlag string, rootToNewLeafTablesMap map[string][]string, rootTables []sqlname.NameTuple, firstRunTableWithLeafsAndRoots []sqlname.NameTuple) ([]sqlname.NameTuple, []sqlname.NameTuple, 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.
nit: new lines for the args. Hard to read otherwise.
return true, nil | ||
} | ||
|
||
func (reg *NameRegistry) GetRegisteredTableList() ([]*sqlname.ObjectName, 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.
add a comment here on why we implemented this (partitions)
yb-voyager/cmd/exportData_test.go
Outdated
} | ||
} | ||
|
||
func fetchTableListFromSourceAndAssertResult(t *testing.T, expectedPartitionsToRootMap map[string]string, expectedTableList []sqlname.NameTuple) { |
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.
this is more like to assert that filtering is happening properly, right? maybe name it something like: assertTableListFiltering
?
yb-voyager/cmd/exportData_test.go
Outdated
assert.Equal(t, len(diff), 0) | ||
} | ||
|
||
func getInitialTableistAndAssertExpectedResult(t *testing.T, expectedTableList []sqlname.NameTuple, expectedPartitionsToRootMap map[string]string) { |
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.
nit: would be good to have these function names start with assert
.
assertInitialTableList
// Tests the unknown table case by over ridding the utils.ErrExit function to assert the error msg | ||
func testUnknownTableCaseForTableListFlags(t *testing.T, expectedUnknownErrorMsg string) { | ||
previousFunction := utils.ErrExit | ||
//changing the error exit function to test the unknown table scenario |
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.
oo, this does not seem like the best way to do it. To be fair, it essentially is just like "mocking" a function, so it's okay, but I think the right way would be:
If we write our functions in a way such that they return a list of guardrail failures, then we can just assert that, instead of having to mocking the errExit function.
Is this too much work given our current state?
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.
It's not a lot of work. I think it's more of a choice between these ways -
- Either we directly display the problem/guardrail and error out
- Return as an error and then display it on the console eventually with the whole context.
e.g.
➜ yb-voyager export data ... --export-type snapshot-and-changes --table-list asdfa
migrationID: a1194e55-3d95-4d37-95e3-d5221eed976d
....
Unknown table names in the include list: [asdfa]
Valid table names are: [...]
➜ yb-voyager export data ... --export-type snapshot-and-changes --table-list asdfa
migrationID: a1194e55-3d95-4d37-95e3-d5221eed976d
...
error getting initial table list: error applying table list flags for current list and remove roots: error in apply table list filter on registered list for the flags in current run: Unknown table names in the include list: [asdfa]
Valid table names are: [...]
I think the first one is better, where we directly display the problem/guardrails because it's not an actual error per say.
Let me know what do you think?
yb-voyager/cmd/exportData_test.go
Outdated
} | ||
defer testPostgresSource.DB().Disconnect() | ||
defer testPostgresSource.ExecuteSqls(cleanUpSqls...) | ||
defer os.RemoveAll(fmt.Sprintf("%s/", testExportDir)) |
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.
remove the sprintf? It's also a bit dangerous in case testExportDir is empty, cause then it will attempt to removeall("/")
t.Errorf("error initialising name reg for the source: %v", err) | ||
} | ||
//Running the command level functions | ||
source = *testPostgresSource.Source |
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.
why do you need to dereference here, a little confused? can't you directly use the pointer, and update the table-list, etc?
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.
source (the global variable) is not a pointer type, so need to dereference here to assign the test source to the actual source
variable that is used by the functions.
yb-voyager/cmd/exportData.go
Outdated
} | ||
|
||
if source.TableList == "" && source.ExcludeTableList == "" { | ||
//which mean no table-lists are passed in subsequent runs |
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.
what if
- in first run, user passed table-list
- in subsequent run, user did not pass table-list
? Shouldn't we report the discrepancy ?
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.
Hmm, okay let me remove this condition altogether and let the guardrails function do its of job of reporting the discrepancy.
yb-voyager/cmd/exportData_test.go
Outdated
} | ||
|
||
// getInitialTableList for subsequent run with start-clean false and basic without table-list flags so no guardrails | ||
startClean = false |
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.
let's make startClean a param of the getInitialTableistAndAssertExpectedResult
func?
assertGuardrailsChecksForMissingAndExtraTablesInSubsequentRun(t, expectedMissingTables2, expectedExtraTables2, firstRunTableList, rootTables) | ||
getInitialTableistAndAssertExpectedResult(t, firstRunTableList, firstRunPartitionsToRootMap) | ||
|
||
//case5: getInitialTableList for subsequent run with start-clean false and basic with table-list and exclude-table-list flags |
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.
is passing both include and exclude list allowed today? i would have thought we should disallow it? in what situations does this help?
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.
yes we allow that, it helps in scenario where:
tables on source - ab1,abc2,abc3,abc4,abc5, ..(few other tables)
table-list abc* (meaning all the abc prefixed tables)
exclude-table-list abc5 (excluding the abc5)
|
||
} | ||
|
||
func testCasesWithDifferentTableListFlagValuesTest1(t *testing.T, firstRunTableList []sqlname.NameTuple, firstRunPartitionsToRootMap map[string]string) { |
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.
nice different set of test-cases!
If/when Shubham finds some other cases, pls do add it here.
|
||
assertGuardrailsChecksForMissingAndExtraTablesInSubsequentRun(t, expectedMissingTables4, expectedExtraTables4, firstRunTableList, rootTables) | ||
getInitialTableistAndAssertExpectedResult(t, firstRunTableList, firstRunPartitionsToRootMap) | ||
} |
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.
test idea: ensure that
first run: foo*
(foo1, foo2)
second run : foo1,foo2
does not result in any guardrails thrown.
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.
yeah, I had it in mind for adding more tests around glob pattern support in the table list flags, will add it in a separate PR.
Describe the changes in this pull request
export data from source
we are now using the stored table list in MSRTableListExportedFromSource
andSourceExportedTableListWithLeafPartitions
based on the source type as for PostgreSQL source we allow filtering the leaf partitions as well so storing them in MSR.export data from target
we are using the stored table list in MSRTableListExportedFromSource
and then populate the leaf partitions from target for all the partitioned tables and then store it in theTargetExportedTableListWithLeafPartitions
for further runs to avoid the fetching new leaf partitions from target.table-list
/exclude-table-list
flags for checking if there is any discrepancy in these on resumption compared to initial lists.Unknown tables found
.Missing tables ...
/Extra tables found ...
than the initial list and give a prompt to the user if they are fine to continue with the initial list, else abort the command and let them restart.Describe if there are any user-facing changes
Yes, as mentioned above there will be user-facing changes for some guardrails around table-list etc.. now user won't be able to change the table-list during the live migration
Behaviour in these scenarios
With or without table-list flags: export data from source/target commands will be unaware of newly added tables and won’t consider it either during the run or re-run.
If a newly added table is passed through the table-list/exclude-table-list - commands error out saying unknown table, valid tables names are […].
With or without table-list flags:
-- During the run, export data from source/target commands will be unaware of newly added leaf tables and won’t consider it.
-- On re-run, export data from source/target commands will report to the user that new leaf tables are detected for partitioned tables with below msg -
If new leaf partition table is passed through the table-list/exclude-table-list flags - commands error out saying unknown table, valid tables names are […].
In case some tables are removed from the initial list in resumption - commands will report that there is discrepancy found and give prompt to user as mentioned below
In case some tables are added from the initial list in resumption - commands will report that there is discrepancy found and give prompt to user as mentioned below
How was this pull request tested?
Added unit tests with test container PG source for various cases.
Some TODOs remaining:
adding test assertions for below cases
Does your PR have changes that can cause upgrade issues?