xds: Reuse filter interceptors across RPCs

[ejona86]

This moves the interceptor creation from the ConfigSelector to the resource update handling.

The code structure changes will make adding support for filter lifecycles (for RLQS) a bit easier. The filter lifecycles will allow filters to share state across interceptors, and constructing all the interceptors on a single thread will mean filters wouldn't need to be thread-safe (but their interceptors would be thread-safe).

CC @larry-safran (It's a bit unclear who best to review; Sergii dug into this some which triggered me to make the PR. If you're interested, take a look.)

[sergiitk]

A few comments/questions on naming and error handling.

[sergiitk]

Should we call it interceptor instead? Reasoning:

1. Term filter is too ambiguous
2. Plural implies a collection. For our purposes, we treat it as a single interceptor (even if combined under-the-hood)

[ejona86]

Interceptors is a type, not a semantic for its purpose. These are the filters we're wanting to run. Yes, their type is interceptor, but they have the semantic of providing xds filter. I purposefully made it plural to be clear that they are all in there. If you think of it as singular then you'll be like, "oh, I need to get a list of interceptors, because there can be multiple filters." I considered keeping it as list just for the sake of clarity, but decided against it.

[sergiitk]

I get the point about not naming integer variables integerFoo. The reason I recommended interceptor instead of filters is not because of the type, but because of the concept. We do have a conceptual entity called interceptors, even a doc page dedicated to it.

I understand that semantically interceptors are filters, but filters is a very loaded term. We have xDS HTTP Filters and in the context of grpc-xds that's what we refer to as filters. Therefore FilterRegistry, FilterConfig, FilterChain, etc.

So my main concern is whoever is going to read this code, will automatically (and wrongly) assume this filters is a list of xds filter instances, or their configs.

Plus, there's a precedent set by XdsServerWrapper.generatePerRouteInterceptors. There we have ImmutableMap.Builder<Route, ServerInterceptor> perRouteInterceptors and List<ServerInterceptor> interceptors.

In my persistent cache feature, I'm adding HashMap<String, Filter> activeFilters and Set<String> filtersToShutdown to the same method. And in this case these are actual filter instances/names. If we renamed perRouteInterceptors and interceptors in XdsServerWrapper.generatePerRouteInterceptors, things would get less readable.

That said, I'll leave the decision to you. Not going to block this PR because of semantic differences in naming approach.

[ejona86]

I agree there is some semantic to an interceptor, but that's true also for Function. It is better, when possible, to describe the specific purposes of specific instances when there are such semantics available.

I do agree with "filter" meaning multiple very-similar-but-not-quite-the-same things. And that's because a filter has specific behavior but the Filter class doesn't actually do that specific behavior and instead you need the interceptor to perform the action. "filters interceptors" is maybe more precise. I preferred the overloaded filters because if you get it wrong, it doesn't hurt much; you get a compile error and a bit annoyed. If you confuse the purpose of the interceptor, then you have a functional issue. Also, on every line the variable is seen there is a strong signal they are interceptors. Having ClientInterceptor filters is awkward, but ClientInterceptor interceptors is asking for rocks to be thrown my way.

"per route interceptors" isn't bad, as it is a scope that implies a purpose. Here we get "per weighted-cluster-unless-it-is-per-route interceptor" nonsense which did annoy me greatly. I did try to avoid the awkward encoding with RouteData by pushing the "filters interceptors" down into the action, but it required duplicating a lot of data structures.

[sergiitk]