http fault: add response rate limit injection #6267
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Part of #5942 Signed-off-by: Matt Klein <mklein@lyft.com>
alyssawilk
reviewed
Mar 12, 2019
Signed-off-by: Matt Klein <mklein@lyft.com>
|
@alyssawilk updated per first comment pass. |
alyssawilk
reviewed
Mar 13, 2019
Signed-off-by: Matt Klein <mklein@lyft.com>
soya3129
reviewed
Mar 13, 2019
soya3129
reviewed
Mar 13, 2019
test/extensions/filters/http/fault/fault_filter_integration_test.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: Matt Klein <mklein@lyft.com>
|
@alyssawilk @soya3129 updated |
rshriram
reviewed
Mar 14, 2019
rshriram
reviewed
Mar 14, 2019
rshriram
reviewed
Mar 14, 2019
| continue_cb_(continue_cb), | ||
| // The token bucket is configured with a max token count of the number of ticks per second, | ||
| // and refills at the same rate, so that we have a per second limit which refills gradually in | ||
| // ~63ms intervals. |
There was a problem hiding this comment.
I think I just remembered something about the backing issue for this PR.. this is bandwidth rate limiting in essence.. but at application layer. No guarantees that the OS isn't buffering it up until MSS and then pushing data [granted there is a timer in the OS, that will cap the wait time]. I used to do this with netem module in the kernel which has a bewildering array of such algorithms, but applied at the netdev level, after TCP packaging. It would be interesting to see if the OS will mask the user space bandwidth throttling or not..
There was a problem hiding this comment.
also how did you come up with the 63ms figure?
There was a problem hiding this comment.
re: your first comment, if there is a concern can you restate?
re: 63ms, 1000ms / 16 (see other comments).
rshriram
reviewed
Mar 14, 2019
alyssawilk
previously approved these changes
Mar 14, 2019
Signed-off-by: Matt Klein <mklein@lyft.com>
|
@rshriram @alyssawilk @soya3129 updated |
soya3129
reviewed
Mar 15, 2019
| upstream_request_->encodeData(data, false); | ||
| decoder->waitForBodyData(1024); | ||
|
|
||
| // Advance time and wait for a ticks worth of data. |
soya3129
reviewed
Mar 15, 2019
| simTime().sleep(std::chrono::milliseconds(63)); | ||
| decoder->waitForBodyData(1088); | ||
|
|
||
| // Advance time and wait for a ticks worth of data. |
soya3129
reviewed
Mar 15, 2019
| simTime().sleep(std::chrono::milliseconds(63)); | ||
| decoder->waitForBodyData(1088); | ||
|
|
||
| // Advance time and wait for a ticks worth of data and end stream. |
soya3129
approved these changes
Mar 15, 2019
|
@soya3129 I will fix the comment issues in my next change. |
dnoe
approved these changes
Mar 15, 2019
spenceral
added a commit
to spenceral/envoy
that referenced
this pull request
Mar 20, 2019
* master: (59 commits) http fault: add response rate limit injection (envoyproxy#6267) xds: introduce initial_fetch_timeout option to limit initialization time (envoyproxy#6048) test: fix cpuset-threads tests (envoyproxy#6278) server: add an API for registering for notifications for server instance life… (envoyproxy#6254) remove remains of TestBase (envoyproxy#6286) dubbo_proxy: Implement the routing of Dubbo requests (envoyproxy#5973) Revert "stats: add new BoolIndicator stat type (envoyproxy#5813)" (envoyproxy#6280) runtime: codifying runtime guarded features (envoyproxy#6134) mysql_filter: fix integration test flakes (envoyproxy#6272) tls: update BoringSSL to debed9a4 (3683). (envoyproxy#6273) rewrite buffer implementation to eliminate evbuffer dependency (envoyproxy#5441) Remove the dependency from TimeSystem to libevent by using the Event::Scheduler abstraction as a delegate. (envoyproxy#6240) fuzz: fix use of literal in default initialization. (envoyproxy#6268) http: add HCM functionality required for rate limiting (envoyproxy#6242) Disable mysql_integration_test until it is deflaked. (envoyproxy#6250) test: use ipv6_only IPv6 addresses in custom cluster integration tests. (envoyproxy#6260) tracing: If parent span is propagated with empty string, it causes th… (envoyproxy#6263) upstream: fix oss-fuzz issue envoyproxy#11095. (envoyproxy#6220) Wire up panic mode subset to receive updates (envoyproxy#6221) docs: clarify xds docs with warming information (envoyproxy#6236) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #5942
Risk Level: Low, new feature, opt-in.
Testing: New UT/integration tests.
Docs Changes: Added
Release Notes: Added