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

http: refactor out stream rate limiter to common #14828

Merged
merged 9 commits into from
Apr 20, 2021

Conversation

nitgoy
Copy link
Contributor

@nitgoy nitgoy commented Jan 26, 2021

Commit Message: Refactor out stream rate limiter class from the fault filter to common. This enables the code to be shared with other extensions such as the bandwidth limit filter. The change is just simple refactoring and shouldn't affect/be visible to users.
Additional Description:
Risk Level: Low
Testing: All unit tests passed.
Docs Changes: None

Signed-off-by: Nitin <nigoyal@microsoft.com>
@nitgoy nitgoy requested a review from alyssawilk as a code owner January 26, 2021 13:19
@repokitteh-read-only
Copy link

Hi @nitgoy, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #14828 was opened by nitgoy.

see: more, trace.

@nitgoy nitgoy changed the title http: Refactor out stream rate limiter to common http: refactor out stream rate limiter to common Jan 26, 2021
@yanavlasov
Copy link
Contributor

Thanks for the PR. Looks like you need to run ./tools/code_format/check_format.py fix to fix code forma errors.

@nitgoy
Copy link
Contributor Author

nitgoy commented Jan 26, 2021

Thanks for the PR. Looks like you need to run ./tools/code_format/check_format.py fix to fix code forma errors.

I'm having some permission issues running that script today. Working on fixing them.

Signed-off-by: Nitin <nigoyal@microsoft.com>
Signed-off-by: Nitin <nigoyal@microsoft.com>
@nitgoy
Copy link
Contributor Author

nitgoy commented Jan 28, 2021

Thanks for the PR. Looks like you need to run ./tools/code_format/check_format.py fix to fix code forma errors.

@yanavlasov Addressed the format issue. Any idea about the current presubmit (examples) failure? It doesn't seem related to my changes. Looks like disk ran out of space or something.

@ggreenway
Copy link
Contributor

@nitgoy a fix for the verify-examples CI job is in main now; merge main into this PR and it should pass.

@yanavlasov
Copy link
Contributor

yanavlasov commented Feb 5, 2021

Thanks for refactoring this. Would it be possible to extract unit tests for the StreamRateLimiter class from the test/extensions/filters/http/fault/fault_filter_test.cc ? It may need some refactoring as well. Otherwise we now have some common class that is tested by an extension.

@yanavlasov
Copy link
Contributor

/wait

Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 19, 2021
@github-actions
Copy link

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this Mar 26, 2021
nitgoy added 2 commits April 7, 2021 19:21
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
Copy link
Contributor Author

nitgoy commented Apr 8, 2021

Hi @yanavlasov. sorry for the long wait but I just got the chance to finalize the changes. Can you help to reopen the PR? I'm not able to see that option on my side.

@nitgoy nitgoy deleted the streamlimiter branch April 8, 2021 03:01
@nitgoy nitgoy restored the streamlimiter branch April 8, 2021 03:01
@yanavlasov yanavlasov reopened this Apr 8, 2021
@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 8, 2021
nitgoy added 2 commits April 8, 2021 13:11
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@yanavlasov yanavlasov merged commit 795a4f5 into envoyproxy:main Apr 20, 2021
gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 6, 2021
Signed-off-by: Nitin <nigoyal@microsoft.com>
Signed-off-by: Gokul Nair <gnair@twitter.com>
@nitgoy nitgoy deleted the streamlimiter branch May 6, 2021 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants