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?
2 participants
Implementation of A103