api, core: make channel logger accessible through NameResolver.Args #6430
Conversation
voidzcy
force-pushed
the
api/access_channel_logger_on_name_resolver_args
branch
from
ebdc510
to
a18eca8
Compare
| @@ -408,22 +408,39 @@ public ConfigOrError parseServiceConfig(Map<String, ?> rawServiceConfig) { | |||
| */ | |||
| @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1770") | |||
| public static final class Args { | |||
| private static final ChannelLogger NOOP_CHANNEL_LOGGER = new ChannelLogger() { | |||
There was a problem hiding this comment.
I'm a bit concerned about a logger that does nothing. I wonder if it would be better to just have it be null.
There was a problem hiding this comment.
I agree, because the javadoc says "Information logged here goes to Channelz, and to the Java logger of this class as well."
There was a problem hiding this comment.
This was something I hesitated on, as said in #6430 (comment).
| @@ -568,6 +597,16 @@ public Builder setServiceConfigParser(ServiceConfigParser parser) { | |||
| return this; | |||
| } | |||
|
|
|||
| /** | |||
| * See {@link Args#getChannelLogger}. This is an optional field. | |||
There was a problem hiding this comment.
Why does this need to be optional?
There was a problem hiding this comment.
I feel like the requirement is too strong for user that must provide a ChannelLogger. So if it is not set, it will just default to a no-op channel logger. Do you think it's better to make this required?
There was a problem hiding this comment.
gRPC is the provider of the Args in most, if not all of the cases. Users of this API are on the consumer side, and they will appreciate a guaranteed non-null ChannelLogger, rather than having to check for null (or wonder whether it's a no-op) every time they use it.
There was a problem hiding this comment.
I don't think we need to call out it is an optional field. We will always set it and if their test doesn't they will just get a NPE. I don't think we should have validation in the builder to guarantee it is there, but that doesn't mean consumers of it need to check for nullness.
There was a problem hiding this comment.
You two have different opinions... So should we make this required in API?
I would prefer this being required (given that we do not default to no-op channel logger if not set) and provides a non-null guarantee to users. In general, I don't like @Nullable APIs and would like to avoid as much as possible.
There was a problem hiding this comment.
As discussed offline, we are trying to make a "soft requirement" for ChannelLogger in NameResolver.Args. That is, it is only required for NameResolver.Args users that ChannelLogger matters to them.
We do not call out if it is required or optional in builder. Implementors that care about this field know what to do.
TODO: create tracking issue forNameResolver.Args.getChannelLogger()API.