search

envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
24.99k stars 4.81k forks source link

some thoughts on the simplification of factory context #26476

Open wbpcode opened 1 year ago

wbpcode commented 1 year ago

These thoughts stem from some completely unreleated work. I originally intended to simplify the interfaces of ClusterInfo and try to aggregate all HTTP reletated options to a single HttpProtocolOptions. Then, I found that the TransportSocketFactoryContext instance of cluster is prapagated in the multiple-levels call and I did a simplifiation (see #26438).

When I was doing #26438, I found that there are lots of types of factory context. All these factory contexts have some similar interfaces that used to access the server-wide resource. In almost all the cases, these interfaces' impl would be a simple wrapper to the interfaces of a server factory context reference.

I am thinking could we make things simpler, could we just add a serverFactoryContext() to these factory contexts to return a reference. This reference will provides server-wide resource accessing. The type of reference could be ServerFactoryContext or CommonFactoryContext or FactoryContextBase, it depends on the set of interface that we want to expose.

Then we will have a clear delimitation between the server-wide resource and context-wide resource. And these factory contexts needn't inherit and implement the interfaces of CommonFactoryContext or FactoryContextBase.

And by this way, the code should be eaiser to maintian. Take #23903 as an example, in the #23903, we changed the return type of admin() interface. Although this interface is only accually implemented by the server, we still need to change almost every factory context because almost all factory contexts inherit the FactoryContextBase. (From this perspective, composition over inheritance).

wbpcode commented 1 year ago

cc @alyssawilk

alyssawilk commented 1 year ago

I would 100% LOVE to have the contexts simplified. I think most of the factorycontext methods can be removed if they get a server factory context of their own (maybe with some additions to that API). one caveat is to be careful about any changes to init manager and stats scope - the server has its own but many factories use listener-rooted scopes or cluster-rooted scopes so the stats go away as the properties are reloaded via xDS and we don't want to inadvertently change that.

wbpcode commented 1 year ago

I will take the ClusterFactoryContext as the initial experimental object because I had did some code cleanup to the common/upstream recently.

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

wbpcode commented 12 months ago

Pick this up again. Iet's do it gradually.