-
Notifications
You must be signed in to change notification settings - Fork 84
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
update envoy to 82b1f66 #2105
update envoy to 82b1f66 #2105
Conversation
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
Signed-off-by: Snow Pettersen <snowp@lyft.com>
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.
Have you verified the reflection required for envoyproxy/envoy#20169 is confirmed to work on iOS & Android? We use allow_multiple_dns_addresses
for Happy Eyeballs which is one of the reloadable features we could check still works as expected.
Also, it looks like IdleTimeoutTests
is crashing perhaps due to an uncaught exception.
I wouldn't expect there to be any platform specificity with that "reflection" since it's just reading the globally registered flags from absl instead of requiring a separate declaration (cc @alyssawilk). Is there an easy way to check that runtime value still work? Is there test coverage? Re the ios test I think it's just a failing assertion, but the signal handler makes Envoy grab the ios exception and makes it look like a native one, looking into it |
Yep removing Envoy signal handling I get
We should probably disable this for ios tests |
envoyproxy/envoy@d7edf3b this is the problematic change, it seems like changing what idle timeouts look like breaks our heuristic |
last time runtime screwed up we had a test fail so I think it'd detect that particular problem. it'd probably be good to have explicit rather than implicit tests for it though! |
) override_request_timeout_by_gateway_timeout was set in #2105 because import broke some tests. It looks like the issue has been addressed in the interim. CI is clean, but this PR adds some extra C++ tests to cover as well. Risk Level: low Testing: new C++ integration tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Snow Pettersen snowp@lyft.com
Description:
Risk Level: Medium
Testing: n/a
Docs Changes: n/a
Release Notes: n/a