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.

Sign up for GitHub

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

Jump to bottom

store: support maintaining labels in Store #21565

Merged
merged 19 commits into from
Dec 10, 2020
Merged

store: support maintaining labels in Store #21565

merged 19 commits into from
Dec 10, 2020

Conversation

Yisaer
Copy link
Contributor

@Yisaer Yisaer commented Dec 8, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

ref #21540

What is changed and how it works?

Support maintaining store Labels in Store and also create a tikcker job to refresh store information to update labels.

Check List

Tests

  • Unit test

Release note

  • No release note

@Yisaer
Copy link
Contributor Author

Yisaer commented Dec 8, 2020 •
edited
Loading

@lysu @djshow832 PTAL

djshow832
djshow832 reviewed Dec 8, 2020
View reviewed changes
store/tikv/region_cache.go Outdated Show resolved Hide resolved
store/tikv/region_cache.go Outdated Show resolved Hide resolved
store/tikv/region_cache.go Outdated Show resolved Hide resolved
@djshow832 djshow832 requested a review from lysu December 8, 2020 11:43
@Yisaer
Copy link
Contributor Author

Yisaer commented Dec 8, 2020

/run-check_dev_2

nolouch
nolouch reviewed Dec 8, 2020
View reviewed changes
store/tikv/region_cache.go Outdated
@@ -305,10 +307,29 @@ func (c *RegionCache) asyncCheckAndResolveLoop() {
case <-c.notifyCheckCh:
needCheckStores = needCheckStores[:0]
c.checkAndResolve(needCheckStores)
case <-ticker.C:
// refresh store once a minute to update labels
c.markAllNeedCheck()
Copy link
Member

Choose a reason for hiding this comment

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

simpler directlyreResolve all stores.

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 think we should reuse needCheck state

Copy link
Member

@nolouch nolouch Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, needCheck is a way fast update on failed before. for now, periodic update labels is a maintenance behavior，reuse needCheck make code more complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's ok to me. updated.

djshow832
djshow832 reviewed Dec 9, 2020
View reviewed changes
Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 9, 2020
@lysu
Copy link
Contributor

lysu commented Dec 9, 2020

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Dec 9, 2020
ti-srebot
ti-srebot previously approved these changes Dec 9, 2020
View reviewed changes
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 9, 2020
@lysu
Copy link
Contributor

lysu commented Dec 9, 2020

maybe we can simplify Store.storeType logic later, currently storeType value just be a "engine" label

@Yisaer
Copy link
Contributor Author

Yisaer commented Dec 9, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 9, 2020
@Yisaer
Copy link
Contributor Author

Yisaer commented Dec 9, 2020

/run-all-tests

@Yisaer
Copy link
Contributor Author

Yisaer commented Dec 9, 2020

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 21588
  • 21587
  • 21575
  • 21572
  • 21591

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@Yisaer merge failed.

@Yisaer
Copy link
Contributor Author

Yisaer commented Dec 9, 2020

/run-check_dev_2

@Yisaer
Copy link
Contributor Author

Yisaer commented Dec 9, 2020

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 21570
  • 21588
  • 21495
  • 19787
  • 21531

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@Yisaer merge failed.

@Yisaer
Copy link
Contributor Author

Yisaer commented Dec 9, 2020

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 21575

@ti-srebot
Copy link
Contributor

/run-all-tests

@Yisaer
Copy link
Contributor Author

Yisaer commented Dec 10, 2020

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 21324

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@Yisaer merge failed.

@djshow832 djshow832 merged commit 254ee2d into pingcap:master Dec 10, 2020
@Yisaer
Copy link
Contributor Author

Yisaer commented Jan 13, 2021

/label sig/infra

@ti-srebot ti-srebot added the sig/sql-infra SIG: SQL Infra label Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djshow832 djshow832 djshow832 approved these changes

@ti-srebot ti-srebot ti-srebot left review comments

@lysu lysu Awaiting requested review from lysu

@nolouch nolouch Awaiting requested review from nolouch

Assignees
No one assigned
Labels
component/config sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Yisaer @lysu @ti-srebot @nolouch @djshow832