-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
tcp: extending tcp integration test #14451
tcp: extending tcp integration test #14451
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally looks great and I think I understand what's going on, but can you add some motivation for this PR to the description?
TEST_P(TcpProxyIntegrationTest, TcpProxyManyConnections) { | ||
autonomous_upstream_ = true; | ||
initialize(); | ||
int num_connections = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: const
clients.push_back(makeTcpConnection(lookupPort("tcp_proxy"))); | ||
} | ||
if (clients.empty()) { | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like 2/3 of the time this test will do nothing; is that intended?
Do you want to seed the list with a few clients first?
for (int i = 0; i < num_connections; ++i) { | ||
IntegrationTcpClientPtr& tcp_client = clients[i]; | ||
ASSERT_TRUE(tcp_client->write( | ||
"GET / HTTP/1.1\r\nHost: foo\r\nclose_after_response: yes\r\ncontent-length: 0\r\n\r\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems unexpected that we are using HTTP manually in a TCP test. Do you want to comment on what the testing strategy is here?
the motivation was that pinterest thought the new pool was having issues with many connections. turns out it wasn't number of connections but throughput (#14453) but as long as I wrote the tests I think they're definitely a good one to have in our suite. |
Adding some tests to ensure both the old and the new pool can handle many connections, and regression testing behavior as connections are added/used/removed over time. Risk Level: n/a (test only) Testing: yes Docs Changes: no Release Notes: no Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
* master: (30 commits) Deflaked: Guarddog_impl_test (envoyproxy#14475) [fuzz] add fuzz tests for hpack encoding and decoding (envoyproxy#13315) [filters] Prevent a filter from sending local reply and continue (envoyproxy#14416) oauth2: improving coverage (envoyproxy#14479) owners: Change dio email address (envoyproxy#14498) macos build: Fix ninja install (envoyproxy#14495) http: use OptRef helper to reduce some boilerplate (envoyproxy#14361) doc: update test/integration/README.md (envoyproxy#14485) server: wait workers to start before draining parent. (envoyproxy#14319) api: relax inline_string length limitation in DataSource (envoyproxy#14461) oauth: properly stop filter chain when a response was sent (envoyproxy#14476) listener: deprecate use_proxy_proto (envoyproxy#14406) deps: update cel and remove a patch (envoyproxy#14473) preconnect: rename: (envoyproxy#14474) coverage: ratcheting limits (envoyproxy#14472) grpc mux: fix sending node again after stream is reset (envoyproxy#14080) [test] Replace printers_include with printers_lib. (envoyproxy#14442) tcp: nodelay in the new pool (envoyproxy#14453) test: replace mock_methodn macros with mock_method (envoyproxy#14450) tcp: extending tcp integration test (envoyproxy#14451) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Adding some tests to ensure both the old and the new pool can handle many connections, and regression testing behavior as connections are added/used/removed over time.
Risk Level: n/a (test only)
Testing: yes
Docs Changes: no
Release Notes: no