-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Add publish rate limit for each broker to avoid OOM #5710
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
Conversation
|
I am not sure if we really have OOM issue. I think I left a comment on #5513 and trying to understand how can we reproduce it.? |
|
@rdhabalia commented at #5513 with the instructions to reproduce. |
|
Thanks @rdhabalia and @sijie for the attention on this. |
|
run java8 tests |
27eb57f to
c87c619
Compare
|
updated with latest master to resolve conflict. |
|
run java8 tests |
|
ping @rdhabalia :) |
sijie
left a comment
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.
@jiazhai overall looks good.
Can you create a task to follow up a future work - we should consider automatically configuring the max bytes based on the NIC bandwidth that broker detects from the system. Pulsar already has this mechanism detecting the NIC bandwidth for load report. We should piggyback this functionality to re-use that feature.
|
@sijie opened new issue to track it. |
codelipenghui
left a comment
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.
👍
|
@rdhabalia I just replied to your comment in #5513. so this change here is using the mechanism you added for namespace rate limiter. what it basically does is having a rate calculator at broker level and piggyback the rate limiting by checking if publish exceeds a certain rate threshold (both namespace setting and broker setting). It is a pretty simple change that piggyback to your mechanism. why is this a concern to you? |
|
run cpp tests |
|
run integration tests |
Fixes #5513
Motivation
Through #3985, user could set the publish rate for each topic, but the topic number for each broker is not limited, so there is case that a lot of topics served in same broker, and if each topic send too many message, it will cause the messages not able to send to BookKeeper in time, and messages be hold in the direct memory of broker, and cause Broker out of direct memory.
Modifications
Verifying this change
unit test passed.