-
Notifications
You must be signed in to change notification settings - Fork 598
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(source): remove chunk splitting logic in apply_rate_limit
#19826
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
apply_rate_limit
apply_rate_limit
9fc46c4
to
10e0ea4
Compare
1911360
to
489ce16
Compare
eeeebb4
to
486f99c
Compare
ab4d67f
to
2ad77a8
Compare
d986091
to
b1a892c
Compare
2ad77a8
to
1069774
Compare
1069774
to
3f15274
Compare
3f15274
to
bd6796d
Compare
bd6796d
to
b2a8ac1
Compare
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.
rubber stamp
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.
rest LGTM
b2a8ac1
to
58d0c4f
Compare
Signed-off-by: Richard Chien <[email protected]>
58d0c4f
to
e0337bb
Compare
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.
Rest LGTM.
// Compute the required permits of this chunk for rate limiting. | ||
pub fn compute_rate_limit_chunk_permits(&self) -> u64 { | ||
self.capacity() as _ | ||
} |
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.
Can we remove it since there is no limit for burst?
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.
I slightly prefer keeping this, because if we don't have this, we may have difficulty to decide between chunk.capacity()
and chunk.cardinality()
later.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
After #19698, we can expect chunks yielded to
SourceExecutor
s to be smaller thanSourceCtrlOpts::chunk_size
, so we don't need to split chunks inapply_rate_limit
(used byFetchExecutor
,FsSourceExecutor
,SourceBackfillExecutor
andSourceExecutor
) anymore.This PR removes the chunk splitting logic from
apply_rate_limit
.Checklist
Documentation
Release note