Skip to content

Conversation

@jiazhai
Copy link
Member

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

Verifying this change

unit test passed.

@jiazhai jiazhai self-assigned this Nov 20, 2019
@sijie sijie added area/broker type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Nov 20, 2019
@sijie sijie added this to the 2.5.0 milestone Nov 20, 2019
@rdhabalia
Copy link
Contributor

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
Copy link
Member

sijie commented Nov 20, 2019

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

@jiazhai
Copy link
Member Author

jiazhai commented Nov 21, 2019

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

@jiazhai
Copy link
Member Author

jiazhai commented Nov 21, 2019

run java8 tests

@jiazhai jiazhai force-pushed the broker_publish_rate branch from 27eb57f to c87c619 Compare November 21, 2019 03:26
@jiazhai
Copy link
Member Author

jiazhai commented Nov 21, 2019

updated with latest master to resolve conflict.

@jiazhai
Copy link
Member Author

jiazhai commented Nov 21, 2019

run java8 tests
run integration tests

@jiazhai
Copy link
Member Author

jiazhai commented Nov 22, 2019

ping @rdhabalia :)

Copy link
Member

@sijie sijie left a 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.

@jiazhai
Copy link
Member Author

jiazhai commented Nov 25, 2019

@sijie opened new issue to track it.

Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@rdhabalia
Copy link
Contributor

@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
Copy link
Member

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?

@jiazhai
Copy link
Member Author

jiazhai commented Nov 26, 2019

run cpp tests

@jiazhai
Copy link
Member Author

jiazhai commented Nov 26, 2019

run integration tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Publish rate limit on broker to avoid OOM

5 participants