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.

Sign up for GitHub

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

Jump to bottom

quic: splitting into client and server #24513

Merged
merged 2 commits into from Dec 14, 2022
Merged

quic: splitting into client and server #24513

merged 2 commits into from Dec 14, 2022

Conversation

alyssawilk
Copy link
Contributor

@alyssawilk alyssawilk commented Dec 13, 2022 •
edited

Splitting build libraries into client and server variants.
Moving the server variants to be weakly linked in the HCM (via factory) such that they won't be included by E-M

Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
envoyproxy/envoy-mobile#2711

@alyssawilk
quic: splitting into client and server
c47a04e 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #24513 was opened by alyssawilk.

see: more, trace.

@alyssawilk alyssawilk marked this pull request as ready for review December 13, 2022 18:38
RyanTheOptimist
RyanTheOptimist approved these changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for doing this. One FYI question.

source/common/http/codec_client.cc
@@ -14,7 +14,7 @@
#include "source/common/http/utility.h"

#ifdef ENVOY_ENABLE_QUIC
#include "source/common/quic/codec_impl.h"
#include "source/common/quic/client_codec_impl.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no source/common/http/codec_server.cc to correspond to this current file? I guess not. How come?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the client side of Envoy, creating upstream QUIC clients :-P
The server bits are in the HCM, and I turned that into factory code to avoid the direct dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, yeah I get that this particular file (codec_client.cc) is for making upstream connections. But I guess I kinda expected there to be some sort of server-side analog in this directory too. (Like how we have QuicSpdyServerStream vs QuicSpdyClientStream in QUICHE). But I guess, as you say, that's all in the HCM. Fascinating. Thanks!

@alyssawilk alyssawilk enabled auto-merge (squash) December 13, 2022 19:03
RyanTheOptimist
RyanTheOptimist approved these changes Dec 13, 2022
View reviewed changes
source/common/http/codec_client.cc
@@ -14,7 +14,7 @@
#include "source/common/http/utility.h"

#ifdef ENVOY_ENABLE_QUIC
#include "source/common/quic/codec_impl.h"
#include "source/common/quic/client_codec_impl.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, yeah I get that this particular file (codec_client.cc) is for making upstream connections. But I guess I kinda expected there to be some sort of server-side analog in this directory too. (Like how we have QuicSpdyServerStream vs QuicSpdyClientStream in QUICHE). But I guess, as you say, that's all in the HCM. Fascinating. Thanks!

@alyssawilk
Merge branch 'main' into full_quic_refactor
d4c2c45 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk merged commit ebb91e7 into envoyproxy:main Dec 14, 2022
jpsim added a commit that referenced this pull request Dec 14, 2022
@jpsim
Merge remote-tracking branch 'origin/main' into jp-update-envoy-build…
2d8b10b 
…-tools

* origin/main: (59 commits)
  Make IsOkAndHolds matcher work with submatchers (#24498)
  ios: fix platform key value store (#24532)
  make ClusterInfo::traffic_stats_ a unique_ptr, so that later we can lazy-init it later. (#24406)
  quic: splitting into client and server (#24513)
  fixing coverage (#24529)
  ci: Add `examplesOnly` condition (#24465)
  ci: sonatype_nexus_upload.py: remove unused format argument (#24471)
  deps: Bump `build_bazel_rules_apple` -> 1.1.3 (#24527)
  deps: Bump `com_github_nghttp2_nghttp2` -> 1.51.0 (#24525)
  deps: Bump `rules_license` -> 0.0.4 (#24523)
  build(deps): bump sphinxcontrib-httpdomain from 1.8.0 to 1.8.1 in /mobile/docs (#24126)
  build(deps): bump github/codeql-action from 2.1.35 to 2.1.36 (#24473)
  build(deps): bump openpolicyagent/opa from 0.47.2-istio to 0.47.3-istio in /examples/ext_authz (#24514)
  build(deps): bump node from `80844b6` to `2770c78` in /examples/ext_authz/auth/http-service (#24515)
  build(deps): bump abseil-cpp to latest version (#24386)
  xDS: add xDS config tracker extension point (#23485)
  kafka: add shared consumer manager (#24494)
  coverage: Improve test coverage (#24355)
  deps: Bump `rules_python` -> 0.16.1 (#24344)
  ci: revert disable running the Objective-C integration app (#24478)" (#24496)
  ...

Signed-off-by: JP Simard <jp@jpsim.com>
@alyssawilk alyssawilk deleted the full_quic_refactor branch April 5, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RyanTheOptimist RyanTheOptimist RyanTheOptimist approved these changes

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

@ggreenway ggreenway Awaiting requested review from ggreenway ggreenway is a code owner

Assignees

@RyanTheOptimist RyanTheOptimist

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alyssawilk @RyanTheOptimist