A110: Child Channel Options#529
Conversation
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
The overall design looks good, but I think the doc needs work.
My main concern is that we need a much better description of the overall design in a language-agnostic way, rather than just saying that we have a different design for each language. It's true that each language has its own APIs for how per-channel options are set, but the overall goal here is still to have a way to pass a set of options to be used in child channels. The semantics for that should be the same in all languages.
Please let me know if you have any questions. Thanks!
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
This is looking much better!
Please let me know if you have any questions. Thanks!
…g to make it clear
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
Just one small remaining comment, otherwise looks great!
|
|
||
| * Java: The `Helper` will provide a function that accepts a `ChannelBuilder` and applies the child channel options to it. | ||
| * Go: A new field will be added to the `BuildOptions` struct (passed when creating a resolver or LB policy) to contain the child channel options. | ||
| * C-core: No special plumbing is needed because the child channel args are simply passed as channel arguments, which are already available to LB policies. |
There was a problem hiding this comment.
We should also say that the LB policy will need to propagate both the individual child channel args and the arg containing the child channel args to the child channel.
|
|
||
| 1. It retrieves `O_child` from `P`. | ||
| 2. It applies `O_child` to the configuration of `C`. | ||
| 3. It should also configure channel `C` to use `O_child` for its children. |
There was a problem hiding this comment.
In "LB Policies and Resolvers", C-core has it pretty explicit which component is responsible for this ("configure channel C to use O_child"). It isn't clear to me which internal component is responsible in Go and Java. And looking at grpc/grpc-java#12578, I don't see this done at all.
There was a problem hiding this comment.
In GrpcXdsTransportFactory, the constructor for GrpcXdsTransport after configuring the xDS control plane child channel using channelConfigurator, should pass it recursively:
ManagedChannelBuilder<?> channelBuilder = Grpc.newChannelBuilder(target, channelCredentials)
.keepAliveTime(5, TimeUnit.MINUTES);
channelConfigurator.configureChannelBuilder(channelBuilder);
// Missing: channelBuilder.childChannelConfigurator(channelConfigurator);
In ManagedChannelImpl, inside createResolvingOobChannelBuilder helper, out-of-band channels are created and configured, but the configurator is not recursively set:
ResolvingOobChannelBuilder builder = new ResolvingOobChannelBuilder();
// Configures the channel C (the OOB channel)
channelConfigurator.configureChannelBuilder(builder);
// Missing: builder.childChannelConfigurator(channelConfigurator);
8 participants
No description provided.