Skip to content
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

Add publish rate limit for each broker to avoid OOM #5710

Merged
merged 4 commits into from Nov 26, 2019

Conversation

@jiazhai
Copy link
Contributor

jiazhai commented Nov 20, 2019

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

  • add broker publish rate limit base on #3985,
  • add unit test.

Verifying this change

unit test passed.

@jiazhai jiazhai self-assigned this Nov 20, 2019
@sijie sijie added this to the 2.5.0 milestone Nov 20, 2019
@rdhabalia

This comment has been minimized.

Copy link
Contributor

rdhabalia commented Nov 20, 2019

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.?

@sijie

This comment has been minimized.

Copy link
Contributor

sijie commented Nov 20, 2019

@rdhabalia commented at #5513 with the instructions to reproduce.

@jiazhai

This comment has been minimized.

Copy link
Contributor Author

jiazhai commented Nov 21, 2019

Thanks @rdhabalia and @sijie for the attention on this.

@jiazhai

This comment has been minimized.

Copy link
Contributor Author

jiazhai commented Nov 21, 2019

run java8 tests

@jiazhai jiazhai force-pushed the jiazhai:broker_publish_rate branch from 27eb57f to c87c619 Nov 21, 2019
@jiazhai

This comment has been minimized.

Copy link
Contributor Author

jiazhai commented Nov 21, 2019

updated with latest master to resolve conflict.

@jiazhai

This comment has been minimized.

Copy link
Contributor Author

jiazhai commented Nov 21, 2019

run java8 tests
run integration tests

@jiazhai

This comment has been minimized.

Copy link
Contributor Author

jiazhai commented Nov 22, 2019

ping @rdhabalia :)

@sijie
sijie approved these changes Nov 24, 2019
Copy link
Contributor

sijie left a comment

@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.

@jiazhai

This comment has been minimized.

Copy link
Contributor Author

jiazhai commented Nov 25, 2019

@sijie opened new issue to track it.

Copy link
Contributor

codelipenghui left a comment

👍

@rdhabalia

This comment has been minimized.

Copy link
Contributor

rdhabalia commented Nov 25, 2019

@sijie @jiazhai I have gone through the change and I have also put some comments and concern at #5513 . can you check if that makes sense?

@sijie

This comment has been minimized.

Copy link
Contributor

sijie commented Nov 25, 2019

@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?

conf/broker.conf Outdated Show resolved Hide resolved
@jiazhai

This comment has been minimized.

Copy link
Contributor Author

jiazhai commented Nov 26, 2019

run cpp tests

@jiazhai

This comment has been minimized.

Copy link
Contributor Author

jiazhai commented Nov 26, 2019

run integration tests

@jiazhai jiazhai merged commit 9e7b938 into apache:master Nov 26, 2019
3 checks passed
3 checks passed
Jenkins: C++ / Python Tests SUCCESS
Details
Jenkins: Integration Tests SUCCESS
Details
Jenkins: Java 8 - Unit Tests SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.