fix: handle planner_version and planner-version correctly#10534
Merged
systay merged 2 commits intovitessio:mainfrom Jun 17, 2022
Merged
fix: handle planner_version and planner-version correctly#10534systay merged 2 commits intovitessio:mainfrom
systay merged 2 commits intovitessio:mainfrom
Conversation
Signed-off-by: Andres Taylor <andres@planetscale.com>
Contributor
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
GuptaManan100
approved these changes
Jun 17, 2022
| mysqlPort = flag.Int("mysql_port", 3306, "mysql port") | ||
| externalTopoServer = flag.Bool("external_topo_server", false, "Should vtcombo use an external topology server instead of starting its own in-memory topology server. "+ | ||
| "If true, vtcombo will use the flags defined in topo/server.go to open topo server") | ||
| plannerVersion = flag.String("planner-version", "gen4", "Sets the default planner to use when the session has not changed it. Valid values are: V3, Gen4, Gen4Greedy and Gen4Fallback. Gen4Fallback tries the gen4 planner and falls back to the V3 planner if the gen4 fails.") |
Contributor
There was a problem hiding this comment.
This makes a lot of sense. Otherwise users would have provided the old flag, and it would have conflicted with the default in this flag!
Signed-off-by: Andres Taylor <andres@planetscale.com>
frouioui
pushed a commit
to planetscale/vitess
that referenced
this pull request
Jun 28, 2022
…0534) * fix: handle planner_version and planner-version correctly Signed-off-by: Andres Taylor <andres@planetscale.com> * test: clean up test and remove the '(default gen4)' Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
3 tasks
deepthi
pushed a commit
that referenced
this pull request
Jun 28, 2022
) * fix: handle planner_version and planner-version correctly (#10534) * fix: handle planner_version and planner-version correctly Signed-off-by: Andres Taylor <andres@planetscale.com> * test: clean up test and remove the '(default gen4)' Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> * Updated expected output for vtgate and vttablet flags Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> Co-authored-by: Andres Taylor <andres@planetscale.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
I recently tried fixing this situation, and I think I made it worse in that PR. 🤦
So here we go again.
Some applications were using
planner-version, and some were usingplanner_version. This change makes it possible to use either, but either they have to agree, or only one of them set.If the user is using
planner_version, which is the deprecated form, they will get a log message saying they shouldn't use that anymore.Related Issue(s)
Original attempt: #10453
Checklist
Deployment Notes