Add -rename_tables to DropSources
#6383
Conversation
This addition allows the optional bool -rename_tables for DropSources. This can be useful when dropping sources for larger tables. Instead of a `drop table`, a `rename table` is run. This will be faster in most cases but will require the user to clean up the tables at a later date. Signed-off-by: Tom Krouper <[email protected]>
|
Is there a way to re-kick |
|
cc: #6361 (comment) @shlomi-noach I moved the PR change over here from ☝️. I've added a test for the |
|
Heh, didn;t even get a chance to review 🤷♂️ |
|
You can still review, and we'll fix forward :) |
There was a problem hiding this comment.
@tomkrouper I'm so glad you made this PR! I had it on my (short? long? -term) plan to implement a transparent DROP/RENAME/nibble logic, completely within vitess, and this ticks the first step for it.
Looks good! I'd do a minor golang-style change as suggested in review comments, but good to go as it is!
| types := [...]string{ | ||
| "DROP TABLE", | ||
| "RENAME TABLE", | ||
| } |
There was a problem hiding this comment.
Worth extracting the above code outside the method, since it always generates the same. Also, suggesting to make it a map, such that e.g. types[DropTable]="DROP TABLE".
There was a problem hiding this comment.
@shlomi-noach from a logical perspective, it makes total sense to move the code outside a method that gets repeatedly called. However, I don't understand what you mean by "make it a map". I mean, I could figure out how, but I don't get why.
There was a problem hiding this comment.
@tomkrouper yeah, so I believe it would be a more idiomatic way; for example, right now the code runs if trt < DropTable || trt > RenameTable {, but, if we add a new, third option, then we'd need to change trt > RenameTable to trt > SomethingNew ; where in fact we don't care about ranges, but of existence, and that's where maps are idiomatic and their use is widespread.
So, if we have:
mymap := map[string]string{DropTable: "DROP TABLE", RenameTable: "RENAME TABLE"}we can then:
if t, ok := mymap[trt] ; ok {and that answers "is trt known or unknown to us".
Anyway, that's the why the way I understand it, but I may be just biased towards my own habits.
There was a problem hiding this comment.
Makes total sense. Thanks.
| "DROP TABLE", | ||
| "RENAME TABLE", | ||
| } | ||
| if trt < DropTable || trt > RenameTable { |
There was a problem hiding this comment.
following the above map suggestion, the above would turn to:
if t, ok := tableRemovalTypeMap[trt] ; ok {
return t
}
return "Unknown"
This addition allows the optional bool -rename_tables for DropSources.
This can be useful when dropping sources for larger tables. Instead of a
drop table, arename tableis run. This will be faster in most casesbut will require the user to clean up the tables at a later date.
Signed-off-by: Tom Krouper [email protected]