import: relax memory check constraints#18248
Conversation
Signed-off-by: 3pointer <luancheng@pingcap.com>
Signed-off-by: 3pointer <luancheng@pingcap.com>
Signed-off-by: 3pointer <luancheng@pingcap.com>
|
Skipping CI for Draft Pull Request. |
8884a89 to
a9b21c9
Compare
Signed-off-by: 3pointer <luancheng@pingcap.com>
Tristan1900
left a comment
There was a problem hiding this comment.
Thanks for the change! just a couple comments from my side
| SysQuota::memory_limit_in_bytes() | ||
| })(); | ||
|
|
||
| if mem_limit == 0 || mem_limit < usage { |
There was a problem hiding this comment.
it seems mem_limit could be -1 if unlimited when using cgroup. maybe can have better logging for that case?
There was a problem hiding this comment.
mem_limit is u64, so I think it cannot be -1
src/import/sst_service.rs
Outdated
| Ok(()) => (), | ||
| Err(e) => { | ||
| // in case of immediate retry from client side | ||
| tokio::time::sleep(Duration::from_secs(1)).await; |
There was a problem hiding this comment.
should we adjust the backoff strategy on the client side instead of injecting delay on the server side?
holding a bunch of calls in memory for 1 sec could potentially increase memory pressure,
also sleep for 1s possibly will not solve the burst issue but only delay the burst for 1s, if we want to smooth out we need to add a jitter to the delay. I think jitter can already be configured at the backoff strategy on client side.
There was a problem hiding this comment.
Actually client side has a backoff strategy. So I'm not sure that's the root cause. anyway I'll test it
There was a problem hiding this comment.
anyway, add a jitter is more solid, I'll add and test it
|
I think the PR's title contradicts what this PR is actually doing. "Relax memory check constraints" may be a better name. |
src/import/sst_service.rs
Outdated
| // Reject ONLY if BOTH: | ||
| // - Available memory is below REJECT_SERVE_MEMORY_USAGE | ||
| // - Memory usage ratio is 90%+ | ||
| if mem_limit - usage < REJECT_SERVE_MEMORY_USAGE |
There was a problem hiding this comment.
nit: I feel the following version is more readable.
let free_memory = mem_limit - usage;
let min_required_memory = std::cmp::min(
REJECT_SERVE_MEMORY_USAGE,
((1.0 - HIGH_IMPORT_MEMORY_WATER_RATIO) * mem_limit as f64) as u64
);
if free_memory < min_required_memory {
let usage_ratio = usage as f64 / mem_limit as f64;
return Err(Error::ResourceNotEnough(format!(
"Memory usage too high, usage: {} bytes, mem limit {} bytes",
usage,
mem_limit
)));
}
Signed-off-by: 3pointer <luancheng@pingcap.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Leavrth, YuJuncen The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This reverts commit 71aecc2.
ref tikv#18124 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
What is changed and how it works?
Issue Number: ref #18124
What's Changed:
The previously (#18192) implemented memory check was too conservative, leading to unnecessary rejections even when there was still available memory.
For example, on an 8GB node, the memory_usage_limit was calculated as 6GB, and applying the 0.9 threshold effectively resulted in a 5.4GB limit, which is only 67.5% of the total memory. This led to premature rejection of import requests, even when the system still had usable memory.
Related changes
pingcap/docs/pingcap/docs-cn:Check List
Tests
Side effects
Release note