executor,sessionctx: add correctness for system variables #12311

xiaoronglv · 2019-09-23T08:16:42Z

What problem does this PR solve?

For MySQL, thread_pool_size is the number of thread groups, which determine how many queries can execute simultaneously.

This commit is to add constraints for global variable thread_pool_size for TiDB, which is a subtask of epic Complete correctness for system.

What is changed and how it works?

Add restriction for global variable thread_pool_size, of which

the value must be equal or greater than 1
the value must be equal or less than 64

Check List

Tests

Unit test

Code changes

Has exported variable/fields change

Side effects

Possible performance regression
Increased code complexity

## What Add restriction for system variable `thread_pool_size`. ## Reference 1. [MySQL global variable: thread_pool_size](https://dev.mysql.com/doc/refman/5.7/en/server-system-variables.html#sysvar_version_compile_os) 2. [TiDB guide to cover more restrictions on system variable ](pingcap#7195)

CLAassistant · 2019-09-23T08:16:46Z

All committers have signed the CLA.

xiaoronglv · 2019-09-23T08:26:24Z

Hi @spongedu @djshow832 Could you please help to review my code?

Thanks!

zz-jason

LGTM

zz-jason · 2019-09-24T05:53:26Z

@xiaoronglv Thanks for your contribution, please follow the Commit Message and Pull Request Style to reformat the PR title.

xiaoronglv · 2019-09-24T08:15:40Z

Hi, @zz-jason

Thanks for your review. I have changed the title based on your review.

executor/set_test.go

xiaoronglv · 2019-10-08T07:05:15Z

Hi @zz-jason @SunRunAway

This pr has been pending for days. Could you please merge it?

SunRunAway

LGTM.

sre-bot · 2019-10-08T07:11:48Z

/run-all-tests

sre-bot · 2019-10-08T07:15:56Z

/run-all-tests

sre-bot · 2019-10-08T07:22:16Z

@xiaoronglv merge failed.

xiaoronglv · 2019-10-08T07:23:39Z

I am checking the failing test, will fix it soon.

codecov · 2019-10-08T07:37:41Z

Codecov Report

Merging #12311 into master will increase coverage by 0.065%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             master     #12311       +/-   ##
===============================================
+ Coverage   79.8891%   79.9541%   +0.065%     
===============================================
  Files           460        461        +1     
  Lines        102636     103403      +767     
===============================================
+ Hits          81995      82675      +680     
- Misses        14677      14751       +74     
- Partials       5964       5977       +13

lance6716 · 2019-10-08T08:04:09Z

/run-all-tests

xiaoronglv · 2019-10-08T08:11:52Z

@SunRunAway

it seems the hosts, which are used to run test framework, are not stable. That caused test failed.

https://internal.pingcap.net/idc-jenkins/blue/rest/organizations/jenkins/pipelines/tidb_ghpr_unit_test/runs/8952/nodes/66/steps/152/log/?start=0

[2019-10-08T07:18:24.741Z] FAIL: helper_test.go:63: HelperTestSuite.SetUpSuite
[2019-10-08T07:18:24.741Z] 
[2019-10-08T07:18:24.741Z] helper_test.go:136:
[2019-10-08T07:18:24.741Z]     c.Assert(err, IsNil)
[2019-10-08T07:18:24.741Z] ... value *net.OpError = &net.OpError{Op:"listen", Net:"tcp", Source:net.Addr(nil), Addr:(*net.TCPAddr)(0xc0002ad7a0), Err:(*os.SyscallError)(0xc000253a40)} ("listen tcp 127.0.0.1:10100: bind: address already in use")
[2019-10-08T07:18:24.741Z] 
[2019-10-08T07:18:24.741Z] 
[2019-10-08T07:18:24.741Z] ----------------------------------------------------------------------
[2019-10-08T07:18:24.741Z] MISS: helper_test.go:97: HelperTestSuite.TestGetRegionsTableInfo
[2019-10-08T07:18:24.741Z] MISS: helper_test.go:75: HelperTestSuite.TestHotRegion
[2019-10-08T07:18:24.741Z] MISS: helper_test.go:105: HelperTestSuite.TestTiKVRegionsInfo
[2019-10-08T07:18:24.741Z] MISS: helper_test.go:115: HelperTestSuite.TestTiKVStoresStat
[2019-10-08T07:18:24.741Z] OOPS: 0 passed, 1 FAILED, 4 MISSED

lance6716 · 2019-10-08T08:12:41Z

/run-all-tests

xiaoronglv · 2019-10-08T08:19:25Z

@lance6716 Thanks for your help.

sre-bot · 2019-10-08T08:26:20Z

cherry pick to release-3.0 failed

sre-bot · 2019-10-08T08:30:53Z

cherry pick to release-3.1 failed

sre-bot · 2019-10-08T08:34:19Z

cherry pick to release-2.1 failed

zz-jason · 2019-10-10T02:21:49Z

@xiaoronglv Could you help us cherry-pick this commit to the release-2.1, release-3.0 and release-3.1 branches?

xiaoronglv · 2019-10-10T03:44:12Z

Hi @zz-jason ,

sure, I can do it manually.

)

xiaoronglv · 2019-10-10T07:09:02Z

Hi @zz-jason

As you requested, I manually cherry pick this commit from master to the following branches

release-2.1 ❌cherry pick: executor,sessionctx: add correctness for system variables (#12311) #12586
release-3.0 ✔️executor,sessionctx: add correctness for system variables (#12311) #12587
release-3.1 ✔️ executor,sessionctx: add correctness for system variables (#12311) #12588

release-2.1 failed to pick this commit due to missing some key commits, which are not cherry picked before.

d381d84d1b

Regarding this commit, can we ignore release-2.1?

If the answer is yes, I am going to close PR for release-2.1

zz-jason · 2019-10-10T07:26:23Z

Seems we missed some commits which should be cherry picked to release-2.1 as well 😂 @spongedu Could you help us cherry pick #11239 to release-2.1?

zz-jason · 2019-10-10T07:28:09Z

There might be other session variable validation PRs not cherry picked to release-2.1

…12587)

sre-bot added the contribution This PR is from a community contributor. label Sep 23, 2019

xiaoronglv changed the title ~~executor,sessionctx: Add correctness for system variables: thread_pool_size~~ Add correctness for system variables: thread_pool_size Sep 23, 2019

zz-jason reviewed Sep 24, 2019

View reviewed changes

zz-jason added status/LGT1 Indicates that a PR has LGTM 1. type/enhancement The issue or PR belongs to an enhancement. needs-cherry-pick-2.1 labels Sep 24, 2019

zz-jason requested review from SunRunAway and qw4990 September 24, 2019 05:52

xiaoronglv changed the title ~~Add correctness for system variables: thread_pool_size~~ executor,sessionctx: add correctness for system variables Sep 24, 2019

SunRunAway reviewed Sep 24, 2019

View reviewed changes

executor/set_test.go Show resolved Hide resolved

qw4990 removed their request for review September 26, 2019 07:20

zz-jason requested a review from SunRunAway October 8, 2019 06:57

SunRunAway approved these changes Oct 8, 2019

View reviewed changes

SunRunAway added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 8, 2019

Merge branch 'master' into ryan-first-pull-request

1280fca

Merge branch 'master' into ryan-first-pull-request

93a7635

Merge branch 'master' into ryan-first-pull-request

13e0853

zz-jason merged commit 3e8129d into pingcap:master Oct 8, 2019

xiaoronglv pushed a commit to xiaoronglv/tidb that referenced this pull request Oct 10, 2019

executor,sessionctx: add correctness for system variables (pingcap#12311

ca969d4

)

xiaoronglv pushed a commit to xiaoronglv/tidb that referenced this pull request Oct 10, 2019

executor,sessionctx: add correctness for system variables (pingcap#12311

3dfdf4f

)

xiaoronglv pushed a commit to xiaoronglv/tidb that referenced this pull request Oct 10, 2019

executor,sessionctx: add correctness for system variables (pingcap#12311

c8c8d76

)

sre-bot pushed a commit that referenced this pull request Oct 10, 2019

executor,sessionctx: add correctness for system variables (#12311) (#…

8a93d6b

…12587)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor,sessionctx: add correctness for system variables #12311

executor,sessionctx: add correctness for system variables #12311

xiaoronglv commented Sep 23, 2019

CLAassistant commented Sep 23, 2019 •

edited

Loading

xiaoronglv commented Sep 23, 2019

zz-jason left a comment

zz-jason commented Sep 24, 2019

xiaoronglv commented Sep 24, 2019

xiaoronglv commented Oct 8, 2019

SunRunAway left a comment

sre-bot commented Oct 8, 2019

sre-bot commented Oct 8, 2019

sre-bot commented Oct 8, 2019

xiaoronglv commented Oct 8, 2019

codecov bot commented Oct 8, 2019 •

edited

Loading

lance6716 commented Oct 8, 2019

xiaoronglv commented Oct 8, 2019 •

edited

Loading

lance6716 commented Oct 8, 2019

xiaoronglv commented Oct 8, 2019

sre-bot commented Oct 8, 2019

sre-bot commented Oct 8, 2019

sre-bot commented Oct 8, 2019

zz-jason commented Oct 10, 2019

xiaoronglv commented Oct 10, 2019

xiaoronglv commented Oct 10, 2019

zz-jason commented Oct 10, 2019

zz-jason commented Oct 10, 2019

executor,sessionctx: add correctness for system variables #12311

executor,sessionctx: add correctness for system variables #12311

Conversation

xiaoronglv commented Sep 23, 2019

What problem does this PR solve?

What is changed and how it works?

Check List

CLAassistant commented Sep 23, 2019 • edited Loading

xiaoronglv commented Sep 23, 2019

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Sep 24, 2019

xiaoronglv commented Sep 24, 2019

xiaoronglv commented Oct 8, 2019

SunRunAway left a comment

Choose a reason for hiding this comment

sre-bot commented Oct 8, 2019

sre-bot commented Oct 8, 2019

sre-bot commented Oct 8, 2019

xiaoronglv commented Oct 8, 2019

codecov bot commented Oct 8, 2019 • edited Loading

Codecov Report

lance6716 commented Oct 8, 2019

xiaoronglv commented Oct 8, 2019 • edited Loading

lance6716 commented Oct 8, 2019

xiaoronglv commented Oct 8, 2019

sre-bot commented Oct 8, 2019

sre-bot commented Oct 8, 2019

sre-bot commented Oct 8, 2019

zz-jason commented Oct 10, 2019

xiaoronglv commented Oct 10, 2019

xiaoronglv commented Oct 10, 2019

zz-jason commented Oct 10, 2019

zz-jason commented Oct 10, 2019

CLAassistant commented Sep 23, 2019 •

edited

Loading

codecov bot commented Oct 8, 2019 •

edited

Loading

xiaoronglv commented Oct 8, 2019 •

edited

Loading