Skip to content

test: optimize waitForNextUpstreamRequest()#11026

Merged
alyssawilk merged 3 commits intoenvoyproxy:masterfrom
lambdai:flakytest
May 5, 2020
Merged

test: optimize waitForNextUpstreamRequest()#11026
alyssawilk merged 3 commits intoenvoyproxy:masterfrom
lambdai:flakytest

Conversation

@lambdai
Copy link
Contributor

@lambdai lambdai commented May 1, 2020

Commit Message:
//test/integration:protocol_integration_test
After:
Stats over 50 runs: max = 11.8s, min = 1.2s, avg = 4.0s, dev = 4.3s
Before:
Stats over 50 runs: max = 2.6s, min = 1.4s, avg = 2.4s, dev = 0.3s

Signed-off-by: Yuchen Dai silentdai@gmail.com

Additional Description:
Risk Level: LOW
Testing: test
Docs Changes: n/a
Release Notes: n/a

//test/integration:protocol_integration_test
After:
  Stats over 50 runs: max = 11.8s, min = 1.2s, avg = 4.0s, dev = 4.3s
Before:
  Stats over 50 runs: max = 2.6s, min = 1.4s, avg = 2.4s, dev = 0.3s

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

AssertionResult result = AssertionFailure();
for (auto upstream_index : upstream_indices) {
int upstream_index = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the test failures are related to the fact that the original only tests upstreams with the given indices, while your version always checks all upstreams? Also, the original is much more likely to return the first member of upstream_indices with a request than the optimized version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I think you are right. I should iterate over the indexes which is passed. An embarrassing mistake.

@zuercher zuercher self-assigned this May 1, 2020
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
zuercher
zuercher previously approved these changes May 4, 2020
Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thanks. This seems reasonable to me. Tagging in Alyssa to see what she thinks.

alyssawilk
alyssawilk previously approved these changes May 4, 2020
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

That's a great catch, and a nice speed up!
Super minor comment nit both otherwise LGTM!

int upstream_index = 0;
Event::TestTimeSystem& time_system = timeSystem();
auto end_time = time_system.monotonicTime() + connection_wait_timeout;
// Loop over the upstreams until timeout or get a upstream request.
Copy link
Contributor

Choose a reason for hiding this comment

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

rephrase?
Loop over the upstreams until the call times out or an upstream request is received.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
@lambdai lambdai dismissed stale reviews from alyssawilk and zuercher via 9b7f521 May 4, 2020 22:50
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Awesome!

@alyssawilk alyssawilk merged commit 0cb6bbf into envoyproxy:master May 5, 2020
@lambdai lambdai deleted the flakytest branch May 5, 2020 21:36
lambdai added a commit to lambdai/envoy-dai that referenced this pull request Jun 11, 2020
Commit Message:
//test/integration:protocol_integration_test
After:
Stats over 50 runs: max = 11.8s, min = 1.2s, avg = 4.0s, dev = 4.3s
Before:
Stats over 50 runs: max = 2.6s, min = 1.4s, avg = 2.4s, dev = 0.3s

Signed-off-by: Yuchen Dai silentdai@gmail.com

Additional Description:
Risk Level: LOW
Testing: test
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
istio-testing pushed a commit to istio/envoy that referenced this pull request Jun 11, 2020
Commit Message:
//test/integration:protocol_integration_test
After:
Stats over 50 runs: max = 11.8s, min = 1.2s, avg = 4.0s, dev = 4.3s
Before:
Stats over 50 runs: max = 2.6s, min = 1.4s, avg = 2.4s, dev = 0.3s

Signed-off-by: Yuchen Dai silentdai@gmail.com

Additional Description:
Risk Level: LOW
Testing: test
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
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