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

envoy: update to d0befbb & add h2ExtendKeepaliveTimeout #2229

Merged
merged 8 commits into from
Apr 29, 2022

Conversation

jpsim
Copy link
Contributor

@jpsim jpsim commented Apr 29, 2022

Diff: envoyproxy/envoy@03f2376...d0befbb

To Do:

  • Add h2ExtendKeepaliveTimeout option to engine builders
  • Add unit tests validating that h2ExtendKeepaliveTimeout properly sets the http2_delay_keepalive_timeout reloadable feature in the yaml configuration
  • Add release notes
  • Add documentation
  • Verify that the expected code paths from http/2: extend keep alive timeout when frame is received envoy#21077 are hit with the flag on and off

Signed-off-by: JP Simard <jp@jpsim.com>
We'll update this to drain the main host in an upcoming PR, but this
keeps the existing behavior.

Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
@jpsim jpsim changed the title Update Envoy to 583ae63 Update Envoy to d0befbb Apr 29, 2022
@jpsim jpsim marked this pull request as ready for review April 29, 2022 15:29
@jpsim jpsim marked this pull request as draft April 29, 2022 15:57
@jpsim
Copy link
Contributor Author

jpsim commented Apr 29, 2022

Converting back to draft as I add an engine builder configuration for envoyproxy/envoy#21077.

Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: JP Simard <jp@jpsim.com>
@jpsim jpsim changed the title Update Envoy to d0befbb Update Envoy to d0befbb Apr 29, 2022
@jpsim jpsim marked this pull request as ready for review April 29, 2022 16:44
@jpsim jpsim requested a review from Augustyniak April 29, 2022 16:46
@jpsim jpsim changed the title Update Envoy to d0befbb envoy: update to d0befbb & add h2ExtendKeepaliveTimeout Apr 29, 2022
Augustyniak
Augustyniak previously approved these changes Apr 29, 2022
Copy link
Contributor

@Augustyniak Augustyniak left a comment

Choose a reason for hiding this comment

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

Nice! The flag is going to help us to measure the impact of a change. Hopefully it's going to improve things and we will be able to eventually remove h2ExtendKeepaliveTimeout method from the public API (and just have the functionality enabled by default)

test/swift/EngineBuilderTests.swift Outdated Show resolved Hide resolved
Signed-off-by: JP Simard <jp@jpsim.com>
*
* @return This builder.
*/
fun h2ExtendKeepaliveTimeout(h2ExtendKeepaliveTimeout: Boolean): EngineBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, one thing that I noticed is that other methods related to h2 ping are named using h2CONNECTIONKeepalive prefix- if you want to add it that's great, if not I think that it's not a big deal as we are going to remove this method at some point anyway (most probably)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is already pretty verbose that I'd rather not make it even longer.

@jpsim jpsim merged commit 2c92576 into main Apr 29, 2022
@jpsim jpsim deleted the update-envoy-to-583ae63 branch April 29, 2022 17:29
jpsim added a commit that referenced this pull request May 2, 2022
* origin/main:
  Bump rules_apple to 0.34.2 (#2236)
  Bump Lyft Support Rotation (#2232)
  ci: add support for `/retest` command (#2219)
  format: add SwiftLint to check-format script (#2230)
  use 64 bit emulators for test (#2228)
  envoy: update to `d0befbb` & add `h2ExtendKeepaliveTimeout` (#2229)
  Add Ryan Hamilton to OWNERS.md (#2224)
  Android cert verifier: first import from chromium/net (#2222)

Signed-off-by: JP Simard <jp@jpsim.com>
jpsim added a commit that referenced this pull request May 3, 2022
…rtion

* origin/main: (57 commits)
  network: add enableDrainPostDnsRefresh to iOS (#2242)
  envoy: update to em-cherry (#2241)
  network: support draining connections after triggered DNS refresh (#2225)
  Bump rules_apple to 0.34.2 (#2236)
  Bump Lyft Support Rotation (#2232)
  ci: add support for `/retest` command (#2219)
  format: add SwiftLint to check-format script (#2230)
  use 64 bit emulators for test (#2228)
  envoy: update to `d0befbb` & add `h2ExtendKeepaliveTimeout` (#2229)
  Add Ryan Hamilton to OWNERS.md (#2224)
  Android cert verifier: first import from chromium/net (#2222)
  Cleanup: remove the unused stats sink metrics_service (#2220)
  bazel: move back to symbol mapping table files (#2218)
  Add new version history section (#2209)
  build: simplify jnilib copy (#2214)
  ci: Update macOS version to macOS 12 (#2208)
  Update releasing.rst (#2200)
  support: Post Lyft support rotation changes to Slack (#2207)
  Don't run bump_support_rotation GitHub Action on forks (#2204)
  Release 0.4.6 (#2201)
  ...

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