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.

Sign up for GitHub

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

Jump to bottom

[stream] added rate limiter #3553

Merged
merged 5 commits into from
Mar 2, 2021
Merged

[stream] added rate limiter #3553

merged 5 commits into from
Mar 2, 2021

Conversation

JackyWYX
Copy link
Contributor

@JackyWYX JackyWYX commented Feb 26, 2021 •
edited
Loading

Description

This is a small break down PR for #3535. This PR sits on top of #3554

  1. Added a ratelimiter module to limit the rate of stream requests.
  2. The ratelimiter is limiting: 1. global requests per second 2. request per second for each stream.
  3. In upper function call, global rate is set at 50/s, single stream rate is set at 10/s

Update 03/01/2021

Added stream removed handling according to comment. Now this PR is on top of stream manager

@JackyWYX JackyWYX requested review from rlan35 and LeoHChen February 26, 2021 22:14
@JackyWYX JackyWYX self-assigned this Feb 26, 2021
@JackyWYX JackyWYX mentioned this pull request Feb 26, 2021
Closed
LeoHChen
LeoHChen reviewed Feb 26, 2021
View reviewed changes
Copy link
Contributor

@LeoHChen LeoHChen left a comment

Choose a reason for hiding this comment

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

please consider to add prometheus metrics support

p2p/stream/common/ratelimiter/ratelimiter.go Show resolved Hide resolved
@JackyWYX
Copy link
Contributor Author

JackyWYX commented Feb 27, 2021 •
edited
Loading

please consider to add prometheus metrics support

After looking into the prometheus for a while, I think I will add prometheus support after the merge of PRs for the following reason:

The prometheus metrics will not effect the key feature. It's a better-to-have feature.
Prometheus metrics can be added in a separate PR which will make the changes more clean.

So I created an issue to be executed after the review of this entire merge.

@JackyWYX JackyWYX force-pushed the stream_ratelimiter branch from 31c77f9 to 71a4b86 Compare March 1, 2021 10:05
@JackyWYX JackyWYX force-pushed the stream_ratelimiter branch from 71a4b86 to 1c5bfdd Compare March 1, 2021 10:06
@JackyWYX JackyWYX requested review from LeoHChen and rlan35 March 1, 2021 10:08
@JackyWYX
Copy link
Contributor Author

JackyWYX commented Mar 1, 2021

@rlan35 @LeoHChen Added removing stream handling to avoid possible memory leak. Please review again.

@rlan35 rlan35 merged commit e08c2ce into harmony-one:main Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rlan35 rlan35 rlan35 approved these changes

@LeoHChen LeoHChen Awaiting requested review from LeoHChen

Assignees

@JackyWYX JackyWYX

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JackyWYX @rlan35 @LeoHChen