-
Notifications
You must be signed in to change notification settings - Fork 728
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
*: add engine filter to exclude special stores #2426
Conversation
Signed-off-by: nolouch <[email protected]>
No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md |
/run-all-tests |
/rebuild |
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.
rest LGTM
server/schedule/filter/filters.go
Outdated
} | ||
|
||
// NewEngineFilter creates a filter that filters store for special engine. | ||
func NewEngineFilter(scope string, allowUses ...string) Filter { |
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 constraint.Values
be specified directly in the constructor?
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 have adds some comments.
Signed-off-by: nolouch <[email protected]>
server/schedule/filter/filters.go
Outdated
// NewEngineFilter creates a filter that filters out default engine stores. | ||
// By default, all stores that are not marked with a special engine will be filtered out. | ||
// Specify the special engine label if you want to include the special stores. | ||
func NewEngineFilter(scope string, allowSpeicalEngines ...string) Filter { |
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.
There is too much inverse logic and it's hard 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.
any suggestion? It is same as SpecailUseFilter
Signed-off-by: nolouch <[email protected]>
/rebuild |
Signed-off-by: nolouch <[email protected]>
/approve |
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
/approve |
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
/merge |
/run-all-tests |
/merge |
/run-all-tests |
@nolouch merge failed. |
/rebuild |
/merge |
/run-all-tests |
@nolouch merge failed. |
/merge |
/run-all-tests |
It's about dashboard, merge it firstly. |
cherry pick to release-4.0 in PR #2447 |
* *: add engine filter Signed-off-by: nolouch <[email protected]> * add comments Signed-off-by: nolouch <[email protected]> * address comments Signed-off-by: nolouch <[email protected]> * address comments Signed-off-by: nolouch <[email protected]> * address comments Signed-off-by: nolouch <[email protected]> Co-authored-by: nolouch <[email protected]>
Signed-off-by: nolouch [email protected]
What problem does this PR solve?
Fix #2423
AvaiableStore()
always returns flash nodes, so the tikv nodes cannot be used.Release note
presplit
not work within tiflash cluster.