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

configuration: enable h2 ping by default #2270

Merged
merged 7 commits into from
May 12, 2022
Merged

Conversation

Augustyniak
Copy link
Contributor

Description: Enable h2 ping by default. Lyft currently has this functionality enabled to all of users who have Envoy Mobile running. It turned out to be effective when it comes to getting rid of stale connections.
Risk Level: Low, enables well tested functionality by default.
Testing: N/A
Docs Changes: Done
Release Notes: Done

Signed-off-by: Rafal Augustyniak raugustyniak@lyft.com

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Copy link
Contributor

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

Typos. We should also document how to disable now that it's not the default.

Thanks for adding release notes!

library/kotlin/io/envoyproxy/envoymobile/EngineBuilder.kt Outdated Show resolved Hide resolved
library/swift/EngineBuilder.swift Outdated Show resolved Hide resolved
Augustyniak and others added 2 commits May 12, 2022 16:47
Co-authored-by: JP Simard <jp@jpsim.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Co-authored-by: JP Simard <jp@jpsim.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak Augustyniak requested a review from jpsim May 12, 2022 20:52
@Augustyniak
Copy link
Contributor Author

Augustyniak commented May 12, 2022

Typos. We should also document how to disable now that it's not the default.

Great point. Updated the doc. Not sure whether it's OK now - let me know. This API for enabling/disabling H2 ping should be eventually updated to access true/false as opposed to this time interval I think. That's a separate thing tho.

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Copy link
Contributor

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

This is fine as it's documenting existing behavior but we should make this field an optional to make it more obvious how to disable it.

@Augustyniak Augustyniak enabled auto-merge (squash) May 12, 2022 20:59
@Augustyniak Augustyniak merged commit 614c098 into main May 12, 2022
@Augustyniak Augustyniak deleted the enable-h2-ping-by-default branch May 12, 2022 21:06
jpsim added a commit that referenced this pull request May 17, 2022
* origin/main: (97 commits)
  docs: update python packages to work with Python 3.10 (#2286)
  test: adding a cancel test, cleaning up copy-paste code (#2283)
  envoy: update to d88f31b (#2279)
  api: disallow setting 'host' header directly (#2275)
  android: add support for registering a platform KV store (#2134)
  Bump Lyft Support Rotation (#2278)
  tools: Enable the VSCode completion db to use bazelisk if available (#2277)
  Release v0.4.6.20220513-4
  Fix android_release_deploy
  Release v0.4.6.20220513-3
  Release v0.4.6.20220513-2
  net: enable happy eyeballs by default (#2272)
  git: avoid merge conflicts when adding changelog entries (#2273)
  docs: fix sphinx reference mismatch warning (#2274)
  tests: add -Xcheck:jni to kotlin integration tests by default (#2269)
  configuration: enable h2 ping by default (#2270)
  Add version history entries for user-facing changes (#2271)
  configuration: filter unroutable addresses on Android by default (#2267)
  Integrate rules_xcodeproj (#2263)
  Add assert when failing to get_env (#2253)
  ...

Signed-off-by: JP Simard <jp@jpsim.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.

None yet

2 participants