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

Revise the Configuration for SimpleLRUCache #17567

Closed
Yisaer opened this issue Jun 1, 2020 · 6 comments · Fixed by #17532
Closed

Revise the Configuration for SimpleLRUCache #17567

Yisaer opened this issue Jun 1, 2020 · 6 comments · Fixed by #17532
Assignees
Labels

Comments

@Yisaer
Copy link
Contributor

Yisaer commented Jun 1, 2020

Feature Request

Is your feature request related to a problem? Please describe:

In my view, some configurations for SimpleLRUCache is misleading and some are over-designed.

For max-memory, the description in our document misleads and may let user to think max-memory is only for managing SimpleLRUCache memory usage.

However, in our config.example.toml, max-memory is described as the memory quota for a single tidb-server. And in #16777 , and we also let max-memory to control the memory quota as a tidb-server. The difference between the usage and description for max-memory may make user confusion.

#17298 also pointed out this.

Also, as GlobalLRUMemUsageTracker introduced in #16777 , I think the memory-guard-ratio is no longer needed and Capacity is enough for the elimination strategy.

Describe the feature you'd like:

To remove the misleading and unnecessary configuration, I think we can

  1. introduce a new configuration like server-memory-quota for the memory quota for a single tidb-server. config: Revise SimpleLRUCache Configuration  #17532
  2. Fix the annotation in tidb.toml.example and tell the readers that max-memory is only for Plan Cache in tidb.toml.example in all release branches(v2.1, v3.0, v3.1, v4.0)
  3. marked max-memory after v4.0

Describe alternatives you've considered:

Teachability, Documentation, Adoption, Migration Strategy:

@Yisaer Yisaer added the type/feature-request Categorizes issue or PR as related to a new feature. label Jun 1, 2020
@zz-jason
Copy link
Member

zz-jason commented Jun 1, 2020

Hi @dbjoa, from the description above, we are going to abandon memory-guard-ratio in the config file, what's your opinion?

@dbjoa
Copy link
Contributor

dbjoa commented Jun 1, 2020

Hi @zz-jason,
I agree with you if we can control the memory usage of plan cache as we done so far.

@SunRunAway
Copy link
Contributor

SunRunAway commented Jun 1, 2020

  1. remain capacity strategy and removed the memory ratio strategy in SimpleLRUCache.

IMO, the memory ratio strategy should be kept (but disabled by default) for compatibility of the upgraded server.

@Yisaer
Copy link
Contributor Author

Yisaer commented Jun 2, 2020

  1. remain capacity strategy and removed the memory ratio strategy in SimpleLRUCache.

IMO, the memory ratio strategy should be kept (but disabled by default) for compatibility of the upgraded server.

It's ok for me.

@fzhedu
Copy link
Contributor

fzhedu commented Jun 2, 2020

We should set different memory pool: server memory = plan memory + executor memory + cache memory + schema memory + statistics memory+ others, similar to MySQL, Oracle and others.

@Yisaer
Copy link
Contributor Author

Yisaer commented Jun 4, 2020

After discuss with @SunRunAway , we decide still remain memory-guard-ration and use server-mem-quota as its quota instead of max-memory

@SunRunAway SunRunAway added type/bug The issue is confirmed as a bug. and removed type/feature-request Categorizes issue or PR as related to a new feature. labels Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants