-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
placement: refine placement options check #29050
Conversation
…pty options Signed-off-by: xhe <[email protected]>
Signed-off-by: xhe <[email protected]>
Signed-off-by: xhe <[email protected]>
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Signed-off-by: xhe <[email protected]>
Signed-off-by: xhe <[email protected]>
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.
LGTM
tidb> create table t1 (a int) followers=4;
Query OK, 0 rows affected (0.11 sec)
tidb> create placement policy `global` primary_region='us-central1' regions='us-central1,us-west1,asia-northeast1';
Query OK, 0 rows affected (0.08 sec)
Signed-off-by: xhe <[email protected]>
Signed-off-by: xhe <[email protected]>
if len(options.PrimaryRegion) > 0 || len(options.Regions) > 0 || len(options.Schedule) > 0 { | ||
isSyntaxSugar = true | ||
// always prefer the sugar syntax, which gives better schedule results most of the time | ||
isSyntaxSugar := true |
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.
I prefer the old implement and regard single followers=xx
configuration as non-sugar options. It is easier to understand.
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 one is just the reverse. It is same. And I must admit sugar options, now, are really smarter than the non-sugar one. Only if you know what you are doing, can you make a better policy with non-sugar options.
BTW, followers
are allowed in both options, and you can not use it to distinguish whether it is sugar or not.
Signed-off-by: xhe <[email protected]>
Signed-off-by: xhe <[email protected]>
Signed-off-by: xhe <[email protected]>
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 742da59
|
/merge |
@xhebox: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
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.
LGTM
What problem does this PR solve?
Issue Number: close #28767 and #29006
Problem Summary: Fix checks to adapt the RFC requirement.
What is changed and how it works?
XX_CONSTRAINTS
without specifying counts.*REGIONS
are set, or the revert. But you can not omit all options.Check List
Tests
Side effects
Documentation
Release note