fix(memory-manager): should not use prev_memory_stats.streaming_memory_usage #9384
Conversation
Codecov Report
@@ Coverage Diff @@
## main #9384 +/- ##
==========================================
+ Coverage 70.79% 70.82% +0.03%
==========================================
Files 1228 1228
Lines 203569 203456 -113
==========================================
- Hits 144111 144100 -11
+ Misses 59458 59356 -102
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| .arg("--memory-control-policy") | ||
| .arg(&config.memory_control_policy) | ||
| .arg("--streaming-memory-proportion") |
There was a problem hiding this comment.
Can you help to confirm these arguments are not used in cloud yet? @arkbriar
There was a problem hiding this comment.
AFAIK, neither the operator nor the cloud control plane use them.
|
Longevity test: https://buildkite.com/risingwave-test/longevity-kubebench/builds/272#0187ae32-d2b6-4b63-adbf-d42b1b610286 (Run on 3e39550. No actual changes except renaming and removing parameter since then) |
| pub struct JemallocMemoryControl { | ||
| threshold_graceful: usize, | ||
| threshold_aggressive: usize, | ||
| } |
There was a problem hiding this comment.
Not sure whether it's a good idea to put configuration values into a policy (sth. like concrete vs abstract).
There was a problem hiding this comment.
Emmmm I prefer this style a little bit, because these values belong to the policy's config in my mind, and besides, these values don't change in loops
|
The longevity can run correctly. I'll merge it then. But sadly, the fluctuation is not completely solved. After running NexMark (q4,q5,q7,q8) for 2 hours:
After running NexMark (q4,q5,q7,q8) for 15 hours:
|
This is not a big fluctuation as previously 🤔 And maybe the curve is not as smooth as we would like it to be under the preemptive memory allocation. |
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Fix #9383.
This PR also reverts #8253 and #8475 but keeps the
trait MemoryControlPolicy(renamed to "MemoryControl") for possible future usage.The reason of that is, we are not actually ready for such "MemoryControlPolicy" right now. Back to that point when we wrote #8253 and #8475, we didn't realize that these policy cannot work at all without a better memory estimation.
After that, when #8827 merged, things become worse. Code of #8827 mixed up 2 things:
... which made that part of code become quite error-prone. I think #9383 is one of its result.
(As you may see, I renamed
StreamingOnlyPolicytoJemallocMemoryControlto emphasize it is a machanism of measuring rather than policy)In my mind, This is a typical case of over-design or premature design. Thus, I strongly tend to remove these unused code and keep it simple and clear. We can add them back when memory estimation is ready, at which time we should get rid of
JemallocMemoryControland add back "StreamingOnlyPolicy" and "StreamingBatchPolicy".Checklist For Contributors
./risedev check(or alias,./risedev c)Checklist For Reviewers
Documentation
My PR DOES NOT contain user-facing changes.
Click here for Documentation
Types of user-facing changes
Please keep the types that apply to your changes, and remove the others.
Release note