store : add store limit to restrain bad store from occupying too much token limit. #12779

AilinKid · 2019-10-17T04:29:55Z

What problem does this PR solve?

In some cases, when a store is broken in tikv cluster, the store will hang all write requests sent to it, which will cause token limit exhausted in tidb server, consequently leading QPS to decrease sharply.

What is changed and how it works?

Add storeLimit in Store structure, we should do the count on it before sending the request to each store, eventually, control the traffic of each store.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

Code changes

Store : add store limit check before sending request

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Release note

add store limit to restrain bad store from occupying too much token limit.

shenli · 2019-10-17T04:31:12Z

hi @AilinKid, Please refine the description.

AilinKid · 2019-10-17T04:41:28Z

hi @AilinKid, Please refine the description.
ok, done

codecov · 2019-10-17T05:08:53Z

Codecov Report

Merging #12779 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #12779   +/-   ##
===========================================
  Coverage   80.2157%   80.2157%           
===========================================
  Files           471        471           
  Lines        114864     114864           
===========================================
  Hits          92139      92139           
  Misses        15579      15579           
  Partials       7146       7146

AilinKid · 2019-10-30T06:50:23Z

/rebuild

lonng · 2019-10-30T07:12:48Z

config/config.go

@@ -339,6 +340,7 @@ var defaultConf = Config{
 	SplitTable:                   true,
 	Lease:                        "45s",
 	TokenLimit:                   1000,
+	StoreLimit:                   -1,


It's better to set the default value to 0 and make compatible with the old configuration.

lonng · 2019-10-30T07:16:00Z

store/tikv/region_request.go

@@ -15,6 +15,7 @@ package tikv

 import (
 	"context"
+	"github.com/pingcap/tidb/config"


The github.com/** should be placed in an individual group.

Addressed,Done.

lonng · 2019-10-30T07:21:24Z

store/tikv/region_request.go

@@ -178,6 +192,27 @@ func (s *RegionRequestSender) sendReqToRegion(bo *Backoffer, ctx *RPCContext, re
 	return
 }

+func (s *RegionRequestSender) getStoreToken(storeID uint64) error {
+	s.storeLimit.Lock()


I think the lock is too coarse. Maybe we can define a guard field in the store struct and use atomic to reduce contention.

Yes, I have tried to reconstruct the code to decrease granularity of the lock

djshow832 · 2019-10-30T07:22:52Z

store/tikv/region_request.go

@@ -167,7 +170,18 @@ func (s *RegionRequestSender) sendReqToRegion(bo *Backoffer, ctx *RPCContext, re
 	if e := tikvrpc.SetContext(req, ctx.Meta, ctx.Peer); e != nil {
 		return nil, false, errors.Trace(e)
 	}
-	resp, err = s.client.SendRequest(bo.ctx, ctx.Addr, req, timeout)
+	// judge the store limit switch.
+	if atomic.LoadInt32(&config.GetGlobalConfig().StoreLimit) != -1 {


How about "> 0" ?

djshow832 · 2019-10-30T07:28:29Z

store/tikv/region_request.go

+		return nil
+	}
+	s.storeLimit.Unlock()
+	return errors.New("store token is up to the limit")


I think an error number and refined message are needed, such as 'maybe TiKV ...(IP) is unavailable'. Otherwise users don't know what to do in this emergency.

djshow832 · 2019-10-30T07:45:44Z

store/tikv/region_request.go

+	s.storeLimit.Lock()
+	limit, ok := s.storeLimit.limit[storeID]
+	if !ok {
+		limit = atomic.LoadInt32(&config.GetGlobalConfig().StoreLimit)


So if it's up to StoreLimit, and the user wants to raise StoreLimit, it won't work, is it right?
I think it's better in this way:s.storeLimit.limit[storeID] is initialized to 0, and increases each time a request comes in, then compares it to s.storeLimit.limit[storeID].
In this way, StoreLimit can be hot reloaded and work immediately.

djshow832 · 2019-10-30T08:24:40Z

hi @AilinKid, Please refine the description.
ok, done

It's still empty in "What is changed and how it works?"

AilinKid · 2019-10-30T08:53:21Z

hi @AilinKid, Please refine the description.
ok, done

It's still empty in "What is changed and how it works?"

good, done

store/tikv/region_request_test.go

lonng · 2019-11-13T05:52:27Z

store/tikv/region_request.go

+
+}
+
+func (s *RegionRequestSender) releaseStoreToken(st *Store) {


I think st.tokenCount cannot be less than or equal to 0, even if it is really less than 0, it doesn't matter.
So prefer just st.tokenCount.Sub(1). (But it's not a strong opinion)
@djshow832 @bb7133

I think it's ok here. @.@

lonng

LGTM

zimulala · 2019-11-13T08:45:32Z

util/storeutil/store_vars.go

@@ -0,0 +1,8 @@
+package storeutil


Please add a license.

Thx, Addressed.

zimulala · 2019-11-13T08:52:44Z

config/config.toml.example

+# store-limit is used to restrain TiDB from sending request to some stores which is up to the limit.
+# If a store has been up to the limit, it will return error for the successive request in same store.
+# default 0 means shutting off store limit.
+store-limit = 0


Please add a test for it.

Thx, addressed.

zimulala · 2019-11-13T09:00:09Z

sessionctx/variable/varsutil.go

@@ -348,6 +348,8 @@ func ValidateSetSystemVar(vars *SessionVars, name string, value string) (string,
 		return value, ErrWrongValueForVar.GenWithStackByArgs(name, value)
 	case SQLSelectLimit:
 		return checkUInt64SystemVar(name, value, 0, math.MaxUint64, vars)
+	case TiDBStoreLimit:
+		return checkInt64SystemVar(name, value, 0, math.MaxInt64, vars)


Do we need to add an upper limit？

hard to define a valid upper limit (big table scanning can cause store limit increase). now is math.MaxInt64.

AilinKid · 2019-11-13T10:03:50Z

/run-all-tests

zimulala

LGTM

sre-bot · 2019-11-13T10:07:06Z

/run-all-tests

sre-bot · 2019-11-13T10:09:11Z

@AilinKid merge failed.

AilinKid · 2019-11-13T10:45:00Z

/run-all-tests

sre-bot · 2019-11-13T10:50:03Z

cherry pick to release-2.1 failed

sre-bot · 2019-11-13T10:50:29Z

cherry pick to release-3.0 failed

sre-bot · 2019-11-13T10:50:59Z

cherry pick to release-3.1 failed

… token limit. (pingcap#12779)

… token limit. (#12779) (#13524)

… token limit. (pingcap#12779)

sre-bot · 2020-04-07T10:24:25Z

It seems that, not for sure, we failed to cherry-pick this commit to release-2.1. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @AilinKid PTAL.

AilinKid changed the title ~~store : add store limit to restrain bad store fromoccupying~~ store : add store limit to restrain bad store from occupying too much token limit. Oct 17, 2019

AilinKid added the status/DNM label Oct 17, 2019

AilinKid added needs-cherry-pick-2.1 labels Oct 17, 2019

AilinKid force-pushed the tokenLimit branch 2 times, most recently from 0d23ba0 to d4c95e5 Compare October 17, 2019 05:06

AilinKid force-pushed the tokenLimit branch 2 times, most recently from 2895d1e to e81f939 Compare October 18, 2019 03:26

AilinKid removed status/DNM labels Oct 30, 2019

AilinKid requested review from crazycs520, lonng and bb7133 October 30, 2019 06:55

lonng reviewed Oct 30, 2019

View reviewed changes

djshow832 reviewed Oct 30, 2019

View reviewed changes

AilinKid force-pushed the tokenLimit branch from c66a574 to 0caa3c1 Compare October 30, 2019 08:55

bb7133 added type/enhancement The issue or PR belongs to an enhancement. needs-cherry-pick-2.1 and removed needs-cherry-pick-2.1 labels Oct 30, 2019

djshow832 reviewed Oct 31, 2019

View reviewed changes

store/tikv/region_request_test.go Show resolved Hide resolved

lonng reviewed Nov 13, 2019

View reviewed changes

AilinKid added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 13, 2019

zimulala reviewed Nov 13, 2019

View reviewed changes

AilinKid and others added 2 commits November 13, 2019 17:59

add a license

850b350

Merge branch 'master' into tokenLimit

ae7ac37

AilinKid added the component/DDL-need-LGT3 label Nov 13, 2019

zimulala approved these changes Nov 13, 2019

View reviewed changes

zimulala added require-LGT3 Indicates that the PR requires three LGTM. status/LGT3 The PR has already had 3 LGTM. status/can-merge Indicates a PR has been approved by a committer. and removed component/DDL-need-LGT3 status/LGT2 Indicates that a PR has LGTM 2. labels Nov 13, 2019

AilinKid merged commit 4283263 into pingcap:master Nov 13, 2019

AilinKid added a commit to AilinKid/tidb that referenced this pull request Nov 14, 2019

store : add store limit to restrain bad store from occupying too much…

14f4b19

… token limit. (pingcap#12779)

AilinKid mentioned this pull request Nov 14, 2019

store : add store limit to restrain bad store from occupying too much token limit. (#12779) #13466

Closed

AilinKid added a commit to AilinKid/tidb that referenced this pull request Nov 18, 2019

store : add store limit to restrain bad store from occupying too much…

b1d2cba

… token limit. (pingcap#12779)

AilinKid mentioned this pull request Nov 18, 2019

store : add store limit to restrain bad store from occupying too much token limit. (#12779) #13524

Merged

sre-bot pushed a commit that referenced this pull request Nov 18, 2019

store : add store limit to restrain bad store from occupying too much…

949b05b

… token limit. (#12779) (#13524)

XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019

store : add store limit to restrain bad store from occupying too much…

dca8644

… token limit. (pingcap#12779)

AilinKid removed the needs-cherry-pick-2.1 label Apr 9, 2020


		}

		func (s RegionRequestSender) releaseStoreToken(st Store) {

store : add store limit to restrain bad store from occupying too much token limit. #12779

store : add store limit to restrain bad store from occupying too much token limit. #12779

Conversation

AilinKid commented Oct 17, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

shenli commented Oct 17, 2019

AilinKid commented Oct 17, 2019

codecov bot commented Oct 17, 2019 • edited Loading

Codecov Report

AilinKid commented Oct 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djshow832 commented Oct 30, 2019

AilinKid commented Oct 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lonng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AilinKid commented Nov 13, 2019

zimulala left a comment

Choose a reason for hiding this comment

sre-bot commented Nov 13, 2019

sre-bot commented Nov 13, 2019

AilinKid commented Nov 13, 2019

sre-bot commented Nov 13, 2019

sre-bot commented Nov 13, 2019

sre-bot commented Nov 13, 2019

sre-bot commented Apr 7, 2020

AilinKid commented Oct 17, 2019 •

edited

Loading

codecov bot commented Oct 17, 2019 •

edited

Loading

AilinKid commented Oct 30, 2019 •

edited

Loading