-
Notifications
You must be signed in to change notification settings - Fork 594
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
feat(streaming): add memory size limit to exchange #16195
Conversation
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
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.
LGTM. Please do some benchmark & longevity before merging
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
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.
So I changed the receiver behavior to accept all ready messages in a batch and then ACK them once. This should allow ACKs to be replied timely but not too frequently. 🤔
I'm not sure if it works as well as before, but it sounds promising. Let's give it a try.
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
This PR has been open for 60 days with no activity. |
Signed-off-by: Runji Wang [email protected]I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR adds a memory size limit to exchange channels, in order to prevent potential OOM issues. This is a successor of #13775. In that PR we completely replaced row limit by memory size limit, but it has been proven to have a bad impact on performance. Therefore, in this PR, we do not abandon the row limit, but add an additional memory limit. In the implementation, we add a new
bytes
permit which will work in parallel with the other two. The default size limit is arbitrarily set to 1MB.Besides, this PR removes the explicit threshold to send the permit back. In my opinion, ACK can be delayed but should not be delayed forever. So I changed the receiver behavior to accept all ready messages in a batch and then ACK them once. This should allow ACKs to be replied timely but not too frequently. 🤔
Checklist
./risedev check
(or alias,./risedev c
)The nexmark benchmark is running. https://buildkite.com/risingwave-test/nexmark-benchmark/builds/3434#018ebcc4-eaa8-4db8-9675-79f7929e3650
Documentation