-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
api, core: make channel logger accessible through NameResolver.Args #6430
api, core: make channel logger accessible through NameResolver.Args #6430
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
TODO: create tracking issue forNameResolver.Args.getChannelLogger()
API.