Skip to content
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

Merged
merged 10 commits into from
May 22, 2020

Conversation

nolouch
Copy link
Contributor

@nolouch nolouch commented May 18, 2020

Signed-off-by: nolouch nolouch@gmail.com

What problem does this PR solve?

Fix #2423
AvaiableStore() always returns flash nodes, so the tikv nodes cannot be used.

Release note

  • Fix the issue that presplit not work within tiflash cluster.

Signed-off-by: nolouch <nolouch@gmail.com>
@nolouch nolouch added the type/bugfix This PR fixes a bug. label May 18, 2020
@nolouch nolouch added this to the v4.0.0-ga milestone May 18, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 18, 2020

@nolouch
Copy link
Contributor Author

nolouch commented May 18, 2020

/run-all-tests

@nolouch nolouch requested review from disksing and rleungx May 18, 2020 09:53
@nolouch
Copy link
Contributor Author

nolouch commented May 18, 2020

/rebuild

@nolouch nolouch added the needs-cherry-pick-release-4.0 The PR needs to cherry pick to release-4.0 branch. label May 18, 2020
Copy link
Member

@HunDunDM HunDunDM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest LGTM

}

// NewEngineFilter creates a filter that filters store for special engine.
func NewEngineFilter(scope string, allowUses ...string) Filter {
Copy link
Member

@HunDunDM HunDunDM May 18, 2020

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?

Copy link
Contributor Author

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.

@nolouch nolouch added type/bug The issue is confirmed as a bug. and removed type/bugfix This PR fixes a bug. labels May 21, 2020
// 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 {
Copy link
Member

@rleungx rleungx May 21, 2020

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.

Copy link
Contributor Author

@nolouch nolouch May 21, 2020

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

@nolouch
Copy link
Contributor Author

nolouch commented May 22, 2020

/rebuild

Signed-off-by: nolouch <nolouch@gmail.com>
@lhy1024
Copy link
Contributor

lhy1024 commented May 22, 2020

/approve

Copy link
Contributor

@sre-bot sre-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rleungx
Copy link
Member

rleungx commented May 22, 2020

/approve

Copy link
Contributor

@sre-bot sre-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nolouch
Copy link
Contributor Author

nolouch commented May 22, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 22, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 22, 2020

/run-all-tests

@nolouch
Copy link
Contributor Author

nolouch commented May 22, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented May 22, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 22, 2020

@nolouch merge failed.

@nolouch
Copy link
Contributor Author

nolouch commented May 22, 2020

/rebuild

@nolouch
Copy link
Contributor Author

nolouch commented May 22, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented May 22, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 22, 2020

@nolouch merge failed.

@lhy1024
Copy link
Contributor

lhy1024 commented May 22, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented May 22, 2020

/run-all-tests

@nolouch
Copy link
Contributor Author

nolouch commented May 22, 2020

It's about dashboard, merge it firstly.

@nolouch nolouch merged commit c471654 into tikv:master May 22, 2020
@nolouch nolouch deleted the fix-engine-scatter branch May 22, 2020 09:46
@sre-bot
Copy link
Contributor

sre-bot commented May 22, 2020

cherry pick to release-4.0 in PR #2447

jebter pushed a commit that referenced this pull request May 25, 2020
* *: add engine filter

Signed-off-by: nolouch <nolouch@gmail.com>

* add comments

Signed-off-by: nolouch <nolouch@gmail.com>

* address comments

Signed-off-by: nolouch <nolouch@gmail.com>

* address comments

Signed-off-by: nolouch <nolouch@gmail.com>

* address comments

Signed-off-by: nolouch <nolouch@gmail.com>

Co-authored-by: nolouch <nolouch@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-cherry-pick-release-4.0 The PR needs to cherry pick to release-4.0 branch. status/can-merge Indicates a PR has been approved by a committer. type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scatter region is not work when there are TiFlash replicas
5 participants