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

Deprecate config item max-memory and add items server-memory-quota and memory-usage-alarm-ratio #4706

Closed
wants to merge 14 commits into from
Closed

Deprecate config item max-memory and add items server-memory-quota and memory-usage-alarm-ratio #4706

wants to merge 14 commits into from

Conversation

wshwsh12
Copy link
Contributor

@wshwsh12 wshwsh12 commented Oct 18, 2020 •
edited
Loading

What is changed, added or deleted? (Required)

Deprecate config item max-memory and add items server-memory-quota and memory-usage-alarm-ratio

max-memory has been deprecated and use server-memory-quota instead now.
memory-usage-alarm-ratio is introduced in this pr pingcap/tidb#18858.

Which TiDB version(s) do your changes apply to? (Required)

  • master (the latest development version)
  • v4.0 (TiDB 4.0 versions)
  • v3.1 (TiDB 3.1 versions)
  • v3.0 (TiDB 3.0 versions)
  • v2.1 (TiDB 2.1 versions)

What is the related PR or file link(s)?

  • This PR is translated from:
  • Other reference link(s):

Do your changes match any of the following descriptions?

  • Delete files
  • Change aliases
  • Have version specific changes
  • Might cause conflicts

@yikeke yikeke added needs-cherry-pick-4.0 status/PTAL This PR is ready for reviewing. translation/doing This PR’s assignee is translating this PR. labels Oct 19, 2020
@yikeke yikeke requested a review from XuHuaiyu October 19, 2020 03:11
yikeke
yikeke reviewed Oct 19, 2020
View reviewed changes
tidb-configuration-file.md Outdated Show resolved Hide resolved
@wshwsh12 @yikeke
Update tidb-configuration-file.md
b81709b 
Co-authored-by: Keke Yi <40977455+yikeke@users.noreply.github.com>
yikeke
yikeke approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@yikeke yikeke left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 20, 2020
@yikeke
Copy link
Contributor

yikeke commented Oct 20, 2020

@wshwsh12 Please help request a tech review.

XuHuaiyu
XuHuaiyu reviewed Oct 21, 2020
View reviewed changes
tidb-configuration-file.md Outdated

+ TiDB 内存使用报警阈值。
+ 默认值：0.8
+ 该配置项的有效范围为 0 到 1。 如果配置该选项为 0 或 1，则表示关闭内存阈值报警功能。否则，当 TiDB 检测到内存使用超过了阈值，则会将相关信息记录到目录 `tmp-storage-path/record` 中。
Copy link
Contributor

@XuHuaiyu XuHuaiyu Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

  1. 这个太简单了，得写下会记录哪些信息, 日志中可能会打印什么内容。
  2. 此外，关于tmp-storage-path 得加个链接到对应的文档项上去
  3. server-memory-quota 也需要加个链接到对应的文档项上
  4. system memory size 换成中文

@wshwsh12 wshwsh12 requested a review from XuHuaiyu October 21, 2020 09:09
@yikeke
Copy link
Contributor

yikeke commented Oct 22, 2020

Please resolve the conflicts. @wshwsh12

CharLotteiu
CharLotteiu reviewed Oct 22, 2020
View reviewed changes
tidb-configuration-file.md Outdated
+ TiDB 内存使用报警阈值。
+ 默认值：0.8
+ 该配置项的有效范围为 0 到 1。 如果配置该选项为 0 或 1，则表示关闭内存阈值报警功能。否则，当 TiDB 检测到内存使用超过了阈值，我们认为 TiDB 目前存在 OOM 的风险，会将当前正在执行的所有 SQLs 中内存使用最高的 10 条 SQL 和运行时间最长的 10 条 SQL 以及 heap profile 记录到目录 [`tmp-storage-path/record`](/tidb-configuration-file.md#tmp-storage-path) 中,并输出一条包含关键子 `the Tidb instance has the risk of OOM` 的日志。
+ 注意：如果配置项 [`server-memory-quota`](/tidb-configuration-file.md#server-memory-quota) 被设置且大于 0，则内存报警阈值将为 `memory-usage-alarm-ratio * server-memory-quota`；否则，内存报警阈值将为 `memory-usage-alarm-ratio * 系统内存大小`。
Copy link
Contributor

Choose a reason for hiding this comment

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

此处提到内存报警阈值为 memory-usage-alarm-ratio 与 server-memory-quota 两个配置项的值的乘积，但第 294 行对 memory-usage-alarm-ratio 这个配置项的解释也是“TiDB 内存使用报警阈值”，前后有点矛盾。而从配置项名称中的“ratio”来看，应该这个配置项的值规定的是内存使用达到何种比例时报警，是否考虑修改一下第 294 行对配置项的说明？

yikeke
yikeke reviewed Oct 22, 2020
View reviewed changes
tidb-configuration-file.md Outdated Show resolved Hide resolved
@ti-srebot
Copy link
Contributor

@yikeke, @XuHuaiyu, @CharLotteiu, PTAL.

1 similar comment
@ti-srebot
Copy link
Contributor

@yikeke, @XuHuaiyu, @CharLotteiu, PTAL.

@yikeke yikeke added the status/require-change Needs the author to address comments. label Oct 26, 2020
@CharLotteiu
Copy link
Contributor

LGTM. @yikeke PTAL.

yikeke
yikeke reviewed Oct 27, 2020
View reviewed changes
Copy link
Contributor

@yikeke yikeke left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@wshwsh12 PTAL. 我重新组织了下这三段的逻辑，希望这样更清楚些：

tidb-configuration-file.md Outdated Show resolved Hide resolved
tidb-configuration-file.md Outdated Show resolved Hide resolved
yikeke
yikeke reviewed Oct 27, 2020
View reviewed changes
tidb-configuration-file.md Outdated Show resolved Hide resolved
wshwsh12 and others added 3 commits October 27, 2020 14:57
@wshwsh12 @yikeke
Update tidb-configuration-file.md
7c31e5a 
Co-authored-by: Keke Yi <40977455+yikeke@users.noreply.github.com>
@wshwsh12 @yikeke
Update tidb-configuration-file.md
d2c69c0 
Co-authored-by: Keke Yi <40977455+yikeke@users.noreply.github.com>
@wshwsh12 @yikeke
Update tidb-configuration-file.md
c7bf12d 
Co-authored-by: Keke Yi <40977455+yikeke@users.noreply.github.com>
@yikeke
Copy link
Contributor

yikeke commented Oct 27, 2020

Note: After pingcap/tidb#20473 is merged, we need a 4.0 cherry-pick of this PR.

yikeke
yikeke approved these changes Oct 27, 2020
View reviewed changes
Copy link
Contributor

@yikeke yikeke left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot
Copy link
Contributor

@wshwsh12, please update your pull request.

@ti-srebot
Copy link
Contributor

@yikeke, @XuHuaiyu, @CharLotteiu, PTAL.

@ti-srebot
Copy link
Contributor

@wshwsh12, please update your pull request.

@ti-srebot
Copy link
Contributor

@yikeke, @XuHuaiyu, @CharLotteiu, PTAL.

@ti-srebot
Copy link
Contributor

@wshwsh12, please update your pull request.

@ti-srebot
Copy link
Contributor

No updates for a long time, close PR.

@ti-srebot
Copy link
Contributor

@yikeke, @XuHuaiyu, @CharLotteiu, PTAL.

@wshwsh12 wshwsh12 closed this Nov 3, 2020
@yikeke
Copy link
Contributor

yikeke commented Nov 3, 2020

Why closed it? @wshwsh12

@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Nov 3, 2020

It needs more information(eg. independent page in troubleshooting) to introduce all memory config.
The bot comments too frequently, so I close this pr now.
I will reopen this pr after I prepare all the information.

@TomShawn TomShawn added the requires-followup This PR requires a follow-up task after being merged. label Nov 5, 2020
@TomShawn TomShawn mentioned this pull request Nov 30, 2020
Merged
10 tasks
@TomShawn TomShawn removed requires-followup This PR requires a follow-up task after being merged. status/LGT1 Indicates that a PR has LGTM 1. status/PTAL This PR is ready for reviewing. status/require-change Needs the author to address comments. translation/doing This PR’s assignee is translating this PR. labels Nov 30, 2020
@ti-srebot ti-srebot mentioned this pull request Dec 2, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CharLotteiu CharLotteiu CharLotteiu left review comments

@yikeke yikeke yikeke approved these changes

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wshwsh12 @yikeke @ti-srebot @CharLotteiu @XuHuaiyu @TomShawn