-
Notifications
You must be signed in to change notification settings - Fork 595
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
refactor: introduce memory control policy abstraction #8253
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
license-eye has totally checked 3006 files.
Valid | Invalid | Ignored | Fixed |
---|---|---|---|
1330 | 1 | 1675 | 0 |
Click to see the invalid file list
- src/compute/src/memory_management/policy.rs
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #8253 +/- ##
==========================================
- Coverage 71.61% 71.57% -0.05%
==========================================
Files 1132 1133 +1
Lines 183410 183507 +97
==========================================
- Hits 131343 131339 -4
- Misses 52067 52168 +101
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM
watermark_epoch: Arc<AtomicU64>, | ||
) -> MemoryControlStats; | ||
|
||
fn describe(&self, total_compute_memory_bytes: usize) -> String; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to pass the unchange parts, e.g. total_compute_memory_bytes
, barrier_interval_ms
into concrete memory control policy implementation so that we can simplify API? For example, this way we don't need the describe
method but implement Display
trait would be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the policy should only contain fields regarding the policy itself while excluding any information about user configurations like the CN memory (though this does make the code a bit more complex).
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR introduces an abstraction for memory control policy:
trait MemoryControl
, and implements this trait onFixedProportionPolicy
(the new memory control policy introduced in #7767) andStreamingOnlyPolicy
(the policy we used before). We could implement this trait on various memory control policies in the future.Related issue: #8228
Checklist For Contributors
./risedev check
(or alias,./risedev c
)Checklist For Reviewers
Documentation
Click here for Documentation
Types of user-facing changes
Please keep the types that apply to your changes, and remove the others.
Release note