WIP: xds mux unification part 2#17049
WIP: xds mux unification part 2#17049dmitri-d wants to merge 21 commits intoenvoyproxy:mainfrom dmitri-d:xds-unification-part-2
Conversation
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
…t.cc Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
| namespace Config { | ||
| namespace UnifiedMux { | ||
|
|
||
| template <class S, class F, class RQ, class RS> |
There was a problem hiding this comment.
@htuch: template base class implementation here
| // This class owns the GrpcStream used to talk to the server, maintains queuing | ||
| // logic to properly order the subscription(s)' various messages, and allows | ||
| // starting/stopping/pausing of the subscriptions. | ||
| template <class S, class F, class RQ, class RS> |
There was a problem hiding this comment.
@htuch: template base class definition here
There was a problem hiding this comment.
Can you reduce the number of template parameters by preferring subclassing relationship for things we directly control, like "state" classes?
There was a problem hiding this comment.
I don't think I can: there's no state interface, for instance, as the base state class is a template with template public methods (getNextRequestAckless and getNextRequestWithAck). B/c state and methods that return state are used in the base mux class, the concrete class must be known at compilation time.
I think this is as small a template-based implementation as it can be, and probably the reason why the original implementation returned pointers to void in getNextRequestAckless and getNextRequestWithAck...
There was a problem hiding this comment.
I just realised I might be able to get away with adding an empty "marker" interface for state classes, and then cast that to an appropriate concrete state class in mux implementations. WDYT, @htuch?
There was a problem hiding this comment.
Not super great, but probably preferable to exploding out template variables. Why is base state a template?
There was a problem hiding this comment.
It basically boils down to DeltaDiscovery[Request/Response]Base and sotw Discovery[Request/Response] not having a common base class. In the original implementation SubscriptionState methods that returned requests/responses or accepted them as parameters were using pointers to void. These have now been changed to templates (here, for example: https://github.com/envoyproxy/envoy/pull/16486/files#diff-ec4538b5bf47eacd7205e605ae59723bbef2a554ac9eae0697a6ab36d1fd1c8fR60).
There was a problem hiding this comment.
I see. I think maybe consider this a request to do what's possible without gymnastics to minimize templates and template variable count within a template where possible. Do the least bad option ;-)
|
Just to be aware, we've been making some improvements to Envoy tooling for more prompt reviews, but draft PRs are not included in the tooling. I'm updating the contribution guidelines to make that clear (https://github.com/envoyproxy/envoy/pull/17063/files) , but as this PR predates the repokitteh warning I just want to call out that draft PRs are likely not going to get fast turnaround. Please consider marking this as ready for review if it stalls, and you want the assignee to take a look! |
|
Closing in favour of #17352 |
4 participants
A follow up to #16486