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

config: Revise SimpleLRUCache Configuration #17532

Merged
merged 12 commits into from
Jun 18, 2020
Merged

config: Revise SimpleLRUCache Configuration #17532

merged 12 commits into from
Jun 18, 2020

Conversation

Yisaer
Copy link
Contributor

@Yisaer Yisaer commented May 29, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number: close #17567

What is changed and how it works?

  1. introduce new configuration to control the mem quota of a single tidb-server
  2. deprecate max-memory configuration
  3. set memory quota for plancache as server-mem-quota instead of max-memory

Release note

  • Add configuration server-memory-quota control the tidb-server memory quota
  • Make max-memory deprecated

@codecov
Copy link

codecov bot commented May 29, 2020 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #17532   +/-   ##
===========================================
  Coverage   79.9105%   79.9105%           
===========================================
  Files           526        526           
  Lines        146505     146505           
===========================================
  Hits         117073     117073           
  Misses        20145      20145           
  Partials       9287       9287

@SunRunAway SunRunAway added the sig/execution SIG execution label May 31, 2020
@Yisaer Yisaer requested a review from SunRunAway June 1, 2020 06:23
SunRunAway
SunRunAway reviewed Jun 1, 2020
View reviewed changes
Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

LGTM

@Yisaer @SunRunAway
Update config/config.toml.example
0d91771 
Co-authored-by: Feng Liyuan <darktemplar.f@gmail.com>
@Yisaer Yisaer requested a review from SunRunAway June 2, 2020 08:11
@Yisaer Yisaer mentioned this pull request Jun 2, 2020
Closed
@SunRunAway
Copy link
Contributor

The PR title has to be changed.

@Yisaer Yisaer changed the title config: fix max-memory meaning config: Revise SimpleLRUCache Configuration Jun 2, 2020
@Yisaer
Copy link
Contributor Author

Yisaer commented Jun 2, 2020

@SunRunAway PTAL when you are free.

@Yisaer
Copy link
Contributor Author

Yisaer commented Jun 8, 2020

/run-all-tests

@Yisaer
Copy link
Contributor Author

Yisaer commented Jun 8, 2020

/run-unit-test

SunRunAway
SunRunAway reviewed Jun 10, 2020
View reviewed changes
Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

LGTM

@SunRunAway SunRunAway added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 10, 2020
@SunRunAway SunRunAway requested a review from qw4990 June 10, 2020 06:43
qw4990
qw4990 reviewed Jun 10, 2020
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

server-mem-quota is also confused, since this config item is for plan cache, why not call it plan-cache-mem-quota?

@Yisaer
Copy link
Contributor Author

Yisaer commented Jun 10, 2020 •
edited
Loading

@qw4990 server-mem-quota means the quota for a single tidb-server as it is the limit for the globa memory tracker. For plan cache, its ratio strategy would be affected by the server-mem-quota instead of max-memory.

The reason why we are going to deprecate the max-memory is that the description for max-memory between our document and config-file comment is conflict(#17298). To ensure none of our users may be confused by the difference, for TiDB 5.0, we will introduce server-mem-quota as the memory limit for a single tidb-server and deprecate the max-memory. For TiDB 4.0 or lower, we will change the max-memory comment to as same as the document described.

@Yisaer Yisaer requested a review from qw4990 June 16, 2020 06:38
qw4990
qw4990 approved these changes Jun 18, 2020
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

@qw4990 qw4990 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 Jun 18, 2020
@ti-srebot ti-srebot requested a review from a team as a code owner June 18, 2020 11:11
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@Yisaer merge failed.

@qw4990
Copy link
Contributor

qw4990 commented Jun 18, 2020

/run-integration-br-test

@qw4990 qw4990 merged commit de6b82e into pingcap:master Jun 18, 2020
@wshwsh12 wshwsh12 mentioned this pull request Oct 19, 2020
Merged
@Yisaer Yisaer mentioned this pull request Dec 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SunRunAway SunRunAway SunRunAway left review comments

@qw4990 qw4990 qw4990 approved these changes

Assignees
No one assigned
Labels
component/config epic/memory-management epic/plan-cache sig/execution SIG execution 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.

Revise the Configuration for SimpleLRUCache
4 participants
@Yisaer @SunRunAway @ti-srebot @qw4990