enable child channel plugins#12578
Conversation
| ObjectPool<XdsClient> getOrCreate( | ||
| String target, BootstrapInfo bootstrapInfo, MetricRecorder metricRecorder, | ||
| ManagedChannel parentChannel); | ||
|
|
There was a problem hiding this comment.
Should this be a generic object instead of ManagedChannel ?
| return new XdsServerWrapper("0.0.0.0:" + port, delegate, xdsServingStatusListener, | ||
| filterChainSelectorManager, xdsClientPoolFactory, bootstrapOverride, filterRegistry); | ||
| filterChainSelectorManager, xdsClientPoolFactory, bootstrapOverride, filterRegistry, | ||
| this.childChannelConfigurer); |
There was a problem hiding this comment.
Why does this not pass the builder object ?
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
I know there will be changes based on the gRFC discussion, but here are some things I noticed.
| @Internal | ||
| protected T addMetricSink(MetricSink metricSink) { | ||
| public T addMetricSink(MetricSink metricSink) { |
There was a problem hiding this comment.
What's happening here? Why will this no longer be internal? Even if we were going to make it public, we wouldn't have it go immediately stable; it'd be experimental first.
There was a problem hiding this comment.
When an application or test implements ChannelConfigurator, it receives a reference to the child channel's ManagedChannelBuilder.
If addMetricSink were protected (or package-private), then class implementations of ChannelConfigurator outside of the io.grpc package (such as user applications or test suites like FakeControlPlaneXdsIntegrationTest.java) would not be able to call builder.addMetricSink(sink) to register their metric sinks.
There was a problem hiding this comment.
What is the problem with that? What new thing do they need to do because of ChannelConfigurator? If addMetricSink() being hidden was a problem, wouldn't that be a pre-existing problem?
GrpcOpenTelemetry.configureChannelBuilder() still works fine with ChannelConfigurator, and it calls addMetricSink() on the user's behalf.
| // NamedFilterConfig.filterStateKey -> filter_instance. | ||
| private final HashMap<String, Filter> activeFiltersDefaultChain = new HashMap<>(); | ||
|
|
||
| private ChannelConfigurator channelConfigurator = new ChannelConfigurator() {}; |
There was a problem hiding this comment.
We really want this field to be final. I see there is some difficulty with the constructors here, but we should handle that with another constructor that has both ScheduledExecutorService and ChannelConfigurator.
(And we should really clean this up by using ObjectPool/SharedResourcePool/FixedObjectPool to get rid of sharedTimeService. But that is pre-existing.)
There was a problem hiding this comment.
I've sent out #12841 to clean that up. Obviously the two PRs will conflict. I'm fine with waiting until this goes in and handling the conflicts. If you want it to go in first to clean up this PR, just tell me.
3 participants
Implements grpc/proposal#529