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

distsql: control the sending rate of copIteratorTaskSender if keepOrder is true #11578

Merged
merged 9 commits into from Aug 7, 2019

Conversation

@SunRunAway
Copy link
Member

commented Aug 2, 2019

What problem does this PR solve?

Closes #11468

What is changed and how it works?

  • Introduce sendRate to control the number of inflight tasks within concurrency * 2.

If keepOrder is true, we must control the sending rate to prevent all tasks being done (aka. all of the responses are buffered) by copIteratorWorker. We keep the number of inflight tasks within the number of concurrency * 2. It sends one more task if a task has been finished in copIterator.Next.

Check List

Tests

  • Manual test (add detailed scripts or steps below)
  • hard to add unit test :(

Code changes

  • None

Side effects

  • None

Related changes

  • Need to cherry-pick to the release branch
@SunRunAway

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

/run-all-tests

@SunRunAway

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

/run-all-tests

@codecov

This comment has been minimized.

Copy link

commented Aug 2, 2019

Codecov Report

Merging #11578 into master will decrease coverage by 0.2399%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##             master     #11578      +/-   ##
==============================================
- Coverage   81.4361%   81.1961%   -0.24%     
==============================================
  Files           427        426       -1     
  Lines         92944      91827    -1117     
==============================================
- Hits          75690      74560    -1130     
- Misses        11872      11899      +27     
+ Partials       5382       5368      -14
@codecov

This comment has been minimized.

Copy link

commented Aug 2, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #11578   +/-   ##
===========================================
  Coverage   81.7785%   81.7785%           
===========================================
  Files           426        426           
  Lines         94345      94345           
===========================================
  Hits          77154      77154           
  Misses        11788      11788           
  Partials       5403       5403

SunRunAway added some commits Aug 2, 2019

fix
fix
@SunRunAway

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

/run-all-tests

@SunRunAway SunRunAway requested review from tiancaiamao, zz-jason and qw4990 Aug 2, 2019

@SunRunAway SunRunAway marked this pull request as ready for review Aug 2, 2019

@SunRunAway SunRunAway changed the title distsql: control sending rate if keepOrder is true distsql: control the sending rate of copIteratorTaskSender if keepOrder is true Aug 2, 2019

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

Use a bare channel makes the code difficult to read, would you like to define a simple rateLimit type:

type rateLimit struct {
     tokens chan struct{}
}

func newRateLimit(n int) *rateLimit {
     return &rateLimit{
	     tokens: make(chan struct{}, n),
     }
}

func (r *rateLimit) getToken() {
	r.tokens <- chan struct{}{}
}

func (r *rateLimit) putToken() {
	<-r.tokens
}

in the sender:

for _, t := range sender.tasks {
	rateLimit.getToken()
	exit := sender.sendToTaskCh(t)
	...
}

and in the worker, when the task is done:

rateLimit.releaseToken()
@zz-jason
Copy link
Member

left a comment

@SunRunAway please address the comments from @tiancaiamao

@SunRunAway

This comment was marked as outdated.

Copy link
Member Author

commented Aug 5, 2019

@tiancaiamao
Since we need to a rateLimit supporting putToken before getToken, it has to do an initialization step for the channel buffer.

So the rateLimit must be implemented like below.

type rateLimit struct {
	token chan struct{}
}

func newRateLimit(initToken, bufLen int) *rateLimit {
	if initToken > bufLen {
		initToken = bufLen
	}
	rl := &rateLimit{
		token : make(chan struct{}, bufLen),
	}
	for i := 0; i < initToken; i++ {
		rl.putToken()
	}
	return rl
}

func (r *rateLimit) getToken(done <-chan struct{}) (exit bool) {
	select {
	case <-done:
		return true
	case <-r.token:
		return false
	}
}

func (r *rateLimit) putToken() {
	r.token<-struct{}{}
}

Do you have any better ideas?

@XuHuaiyu XuHuaiyu self-requested a review Aug 5, 2019

@SunRunAway SunRunAway force-pushed the SunRunAway:issue11468 branch from 45b7623 to afbb8ca Aug 5, 2019

@SunRunAway

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

@tiancaiamao @zz-jason @XuHuaiyu comment addressed, PTAL again, thanks.

@SunRunAway

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

/run-all-tests

@SunRunAway SunRunAway force-pushed the SunRunAway:issue11468 branch from 95e6aca to 0ae7295 Aug 5, 2019

store/tikv/coprocessor.go Show resolved Hide resolved
store/tikv/coprocessor.go Show resolved Hide resolved
@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

LGTM

store/tikv/coprocessor.go Show resolved Hide resolved
store/tikv/coprocessor.go Show resolved Hide resolved
@zz-jason
Copy link
Member

left a comment

LGTM

@sre-bot

This comment has been minimized.

Copy link

commented Aug 6, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link

commented Aug 6, 2019

@SunRunAway merge failed.

@zz-jason

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

/run-auto-merge

@sre-bot

This comment has been minimized.

Copy link

commented Aug 7, 2019

/run-all-tests

@sre-bot sre-bot merged commit 5fff4e9 into pingcap:master Aug 7, 2019

14 checks passed

ci/circleci Your tests passed on CircleCI!
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@sre-bot

This comment has been minimized.

Copy link

commented Aug 7, 2019

cherry pick to release-2.1 failed

@sre-bot

This comment has been minimized.

Copy link

commented Aug 7, 2019

cherry pick to release-3.0 failed

@SunRunAway SunRunAway deleted the SunRunAway:issue11468 branch Aug 7, 2019

sre-bot added a commit to SunRunAway/tidb that referenced this pull request Aug 9, 2019

tiancaiamao added a commit to SunRunAway/tidb that referenced this pull request Aug 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.