composite filter #12646
composite filter #12646
Conversation
AgraVator
force-pushed
the
xds-composite-filter
branch
from
6bf8474 to
603d4fe
Compare
| if (proto.getHttpFiltersList().isEmpty()) { | ||
| throw new ResourceInvalidException("Missing HttpFilter in HttpConnectionManager."); | ||
| } |
There was a problem hiding this comment.
Keeping this the same, assuming we will still be getting Filters here and not all through ExtensionWithMatcherPerRoute
|
@AgraVator Can you split this into two PRs ? 1 for importing protos and another for business logic changes for composite filters? go/small-cls |
| } | ||
|
|
||
| VirtualHost virtualHost = update.getVirtualHost(); | ||
| // filters and there configurations |
| if (proto.getFiltersCount() != 1) { | ||
| throw new ResourceInvalidException("FilterChain " + filterChainName | ||
| + " should contain exact one HttpConnectionManager filter"); | ||
| + " should contain exactly one HttpConnectionManager filter"); |
There was a problem hiding this comment.
nit: Okay for now. But it's usually better to move small unrelated changes into a separate PR.
| static final Attributes.Key<Long> ATTR_DRAIN_GRACE_NANOS = | ||
| Attributes.Key.create("io.grpc.xds.XdsAttributes.drainGraceTime"); | ||
|
|
||
| /** |
There was a problem hiding this comment.
I don't see the usages for these newly added attributes in code, but I can see for others. Is there something I am missing here or will something be added in followup PRs?
| abstract ImmutableMap<String, FilterConfig> filterConfigOverrides(); | ||
|
|
||
| @Nullable | ||
| abstract ImmutableMap<String, com.google.protobuf.Struct> filterMetadata(); |
There was a problem hiding this comment.
go/java-practices/null#collection
Is there a semantic difference between a null filterMetadata and empty filterMetadata ?
| public String[] typeUrls() { | ||
| return new String[] { | ||
| TYPE_URL_EXTENSION_WITH_MATCHER, | ||
| TYPE_URL_COMPOSITE, |
There was a problem hiding this comment.
Why do we need this ?
I'd assume that TypeUrls represent the entry points which in case should be ...matcher or ..matcherperroute . Then we construct the composite filter as needed from the config proto.
Are there cases where we might need this at the top level?
| ExtensionWithMatcherPerRoute proto = any.unpack(ExtensionWithMatcherPerRoute.class); | ||
| matcherProto = proto.getXdsMatcher(); | ||
| } else if (any.is(Composite.class)) { | ||
| return ConfigOrError.fromConfig(new CompositeFilterConfig(null)); |
There was a problem hiding this comment.
Same as above, are there cases we expect a top level compositefilter config ?
| } | ||
|
|
||
| if (matcherProto == null) { | ||
| return ConfigOrError.fromConfig(new CompositeFilterConfig(null)); |
There was a problem hiding this comment.
What does this case cover? any is an arbitrary proto, so we return a default configerror? What does a CompositeFilterConfig with null semantically mean?
| throw new IllegalArgumentException("Failed to unpack TypedStruct", e); | ||
| } | ||
|
|
||
| Filter.Provider provider = FilterRegistry.getDefaultRegistry().get(typeUrl); |
There was a problem hiding this comment.
Usually, depending on a global registry may make testing difficult and brittle, which is somewhat evident by the need to add a visiblefortesting deletion method needed to be added in the FilterRegistry.
My recommendation here would be to follow "program to interface instead of implementation" , by creating an interface and inject it as a dependency into the CompositeFilter .
Then production can use an implementation which can adapt the DefaultFilterRegistry, while the unit test can use a mock.
|
|
||
| @Override | ||
| public String typeUrl() { | ||
| return TYPE_URL_COMPOSITE; |
There was a problem hiding this comment.
Is this the reason for having three entries in the filter provider ? So, that we can correlate the config type to the provider?
| default: | ||
| denominator = 100; | ||
| } | ||
| return ThreadLocalRandom.current().nextInt(denominator) < numerator; |
There was a problem hiding this comment.
nit: Might be okay as it is, but usually having randomness in a concrete class this low in the chain may make it difficult to create deterministic unit tests. go/unit-testing-overview#properties
|
|
||
| return new ServerInterceptor() { | ||
| @Override | ||
| public <ReqT, RespT> io.grpc.ServerCall.Listener<ReqT> interceptCall( |
There was a problem hiding this comment.
nit: Any particular reason for the assymetry? ClientInterceptor is abstracted into a helper class while ServerInterceptor is inline?
| ServerCall<ReqT, RespT> call, Metadata headers, ServerCallHandler<ReqT, RespT> next) { | ||
|
|
||
| // Populate MatchingData with headers and server call attributes | ||
| UnifiedMatcher.MatchingData data = new MatchingDataImpl(headers, null, |
There was a problem hiding this comment.
Maybe we should use go/java-practices/methods#document-ambiguous-arguments to document what the null here is?
| UnifiedMatcher.MatchingData data = new MatchingDataImpl(headers, null, | ||
| call.getAttributes()); | ||
|
|
||
| List<FilterDelegate> delegates = matcher.match(data); |
There was a problem hiding this comment.
Can we use non null empty containers only in the matchers API contract?
| } | ||
| } catch (Throwable t) { | ||
| for (Filter f : filters) { | ||
| f.close(); |
There was a problem hiding this comment.
I am not sure of the solution here due to my unfamiliarity with the language , but we have repeated usages for "close all filters on error" . Is there an alternate design pattern that can do some form of RAII like try-with-resources (or something that allows autocolose on error /scope exit) ?
|
|
||
| delegate.start(responseListener, headers); | ||
|
|
||
| for (Runnable r : pendingEvents) { |
There was a problem hiding this comment.
Why do we need pendingEvents ? Do we expect other methods to be called before start exits?
|
Can we use a typical title that includes a verb and specify the module? Like "xds: Add composite filter". Definitely split out the protos upgrade. If you just added protos in this PR it wouldn't be as hard to look at and as likely to conflict with other changes. But given we're implementing multiple parts of this simultaneously, just adding protos probably should be separate to avoid drift while reviewing (you add a proto here, but someone else does an upgrade, but wouldn't upgrade your new proto). Right now I'm left wondering why you needed to do the upgrade: what field/change are you pulling in? That's the sort of thing useful to describe in the PR/commit description, because there's no way I'll be able to find the specific piece you're wanting. |
3 participants
Implementation of A103