-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Xds mux unification: introduced unifed mux implementation #17352
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
Changes from 25 commits
4993962
c43f23c
3308ced
4a66a31
084a576
b2b5729
916d8ba
6bed794
7289b6e
968b68c
25d7134
6414bf7
8aa55b8
d9e3966
4947a4b
421b56e
93ea6f5
284cdca
1fd1c36
a11a2dc
2450ab8
52a2b1a
d156273
4fabd09
9da89e1
026d42d
c5c8048
e094de6
262f3fc
08e71af
f551585
c564840
b3d0f1c
9219521
2f169b9
a317d84
5dd4162
3568c20
335d7d6
533bb6f
f8178dc
5ac0553
03f7a7c
de40b51
58837b7
2a7a5af
dfe3515
ab0cab5
349de21
cda496e
3777c61
1b5564c
1970420
95075f9
1e58a3d
ba094fb
e169379
893ce72
1ab61ad
bfde33b
cd20489
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -381,5 +381,15 @@ void GrpcMuxImpl::drainRequests() { | |
| grpc_stream_.maybeUpdateQueueSizeStat(request_queue_->size()); | ||
| } | ||
|
|
||
| void NullGrpcMuxImpl::updateWatch(const std::string&, Watch*, | ||
| const absl::flat_hash_set<std::string>&, | ||
| const SubscriptionOptions&) { | ||
| throw EnvoyException("ADS must be configured to support an ADS config source"); | ||
| } | ||
|
|
||
| void NullGrpcMuxImpl::removeWatch(const std::string&, Watch*) { | ||
| throw EnvoyException("ADS must be configured to support an ADS config source"); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: implement these directly in the header file
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We now have a check that yells if exceptions are being raised in headers, which makes me think that such an approach should be used sparingly?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, no exception throwing in headers :) @adisuissa we have an internal requirement around this. |
||
|
|
||
| } // namespace Config | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,12 @@ class GrpcMuxImpl : public GrpcMux, | |
| void setRetryTimer(); | ||
| void sendDiscoveryRequest(const std::string& type_url); | ||
|
|
||
| void removeWatch(const std::string&, Watch*) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } | ||
| void updateWatch(const std::string&, Watch*, const absl::flat_hash_set<std::string>&, | ||
| const SubscriptionOptions&) override { | ||
| NOT_IMPLEMENTED_GCOVR_EXCL_LINE; | ||
| } | ||
|
|
||
| struct GrpcMuxWatchImpl : public GrpcMuxWatch { | ||
| GrpcMuxWatchImpl(const absl::flat_hash_set<std::string>& resources, | ||
| SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder, | ||
|
|
@@ -216,6 +222,9 @@ class NullGrpcMuxImpl : public GrpcMux, | |
| ExceptionUtil::throwEnvoyException("ADS must be configured to support an ADS config source"); | ||
| } | ||
|
|
||
| void updateWatch(const std::string&, Watch*, const absl::flat_hash_set<std::string>&, | ||
| const SubscriptionOptions&) override; | ||
| void removeWatch(const std::string&, Watch*) override; | ||
| void requestOnDemandUpdate(const std::string&, const absl::flat_hash_set<std::string>&) override { | ||
| NOT_IMPLEMENTED_GCOVR_EXCL_LINE; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this not implemented and the other methods throw an exception (ADS must be configured)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I don't feel strongly about this; my thinking was that
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't assume that delta implies ADS though if that's what is suggested.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do think the original message in the exception is weird (see also here: https://github.com/envoyproxy/envoy/blob/main/source/common/config/grpc_mux_impl.h#L215). Perhaps a simple panic would suffice in those places? I could a separate PR to update the message in legacy mux implementation. |
||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.