Conversation
This patch introduces AdsApi, which is where the ADS implementation will go, and AdsSubscription, that matches between the typed Subscription model and untyped AdsApi interface. AdsApi is placed in ClusterManager, despite being only dubiously related to ClusterManager, since it is needed early on and is codependent on ClusterManager. It's also needed at all the xDS instantiation sites where ClusterManager is.
| * is accepted. Accepted configurations have their version_info reflected in subsequent | ||
| * requests. | ||
| */ | ||
| virtual void onConfigUpdate(const Protobuf::RepeatedPtrField<ProtobufWkt::Any>& resources) PURE; |
There was a problem hiding this comment.
Won't the exception cause Envoy to terminate if bad config or some incorrect config arrives? Is that the intent?
There was a problem hiding this comment.
This will work (in AdsApiImpl) similar to https://github.com/lyft/envoy/blob/master/source/common/config/grpc_subscription_impl.h#L95. I.e. we catch the exception and use that to guide the ACK/NAC of the config.
| virtual ~AdsApi() {} | ||
|
|
||
| /** | ||
| * Start a configuration subscription asynchronously for some API type and resources. |
There was a problem hiding this comment.
Dumb question: ADS == LDS+CDS+RDS+EDS right? Or can there be different subsets? Is there a usecase for this subdivision?
There was a problem hiding this comment.
Any xDS API with a ConfigSource in v2 (which is the APIs you point to) can specify to use ADS to mux, see https://github.com/lyft/envoy-api/blob/master/api/base.proto#L192. So, you can use ADS for only a subset. ADS itself will send requests for all 4 APIs, to support push from the management server, but you don't need to ever send an EDS response if EDS doesn't go over ADS. I'll write up more docs/comments in later PRs as this functionality is implemented.
mattklein123
left a comment
There was a problem hiding this comment.
looks good. One question / one nit.
| }; | ||
|
|
||
| class MockAdsWatch : public AdsWatch { | ||
| public: |
There was a problem hiding this comment.
nit: define constructor/destructor in CC file (here and below).
include/envoy/config/ads.h
Outdated
| /** | ||
| * Cancel an ADS watch that was created via AdsApi::subscribe. This will free the AdsWatch object. | ||
| */ | ||
| virtual void cancel() PURE; |
There was a problem hiding this comment.
Any particular reason to not have subscribe() return a unique_ptr and destruction of handle deletes watch? Not a big deal, just curious. Slightly easier for the caller.
| class MockAdsWatch : public AdsWatch { | ||
| public: | ||
| MockAdsWatch(); | ||
| virtual ~MockAdsWatch(); |
There was a problem hiding this comment.
nit: virtual here and below is redundant as base class already has virtual destructor. (Typically we don't do this in code for brevity).
There was a problem hiding this comment.
Will clean this up in following PR.
Description: Add an interface for tracking events that can be used by `EnvoyMobile` to inform platform code (i.e., Android or iOS) about events inside of envoy platform bridge. There are currently no events that are emitted. Risk Level: Low, additive change Testing: Manual, integration test Docs Changes: Done Release Notes: None Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: Add an interface for tracking events that can be used by `EnvoyMobile` to inform platform code (i.e., Android or iOS) about events inside of envoy platform bridge. There are currently no events that are emitted. Risk Level: Low, additive change Testing: Manual, integration test Docs Changes: Done Release Notes: None Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
**Description** This decouples Server struct from the tracer implementation. **Related Issues/PRs (if applicable)** Preparation for #1083 Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
This patch introduces AdsApi, which is where the ADS implementation will go, and AdsSubscription,
that matches between the typed Subscription model and untyped AdsApi interface. AdsApi is placed in
ClusterManager, despite being only dubiously related to ClusterManager, since it is needed early on
and is codependent on ClusterManager. It's also needed at all the xDS instantiation sites where
ClusterManager is.