-
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
Changes from 1 commit
a18eca8
11ba017
495c4ce
1f0d13c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() { | ||
@Override | ||
public void log(ChannelLogLevel level, String message) { | ||
} | ||
|
||
@Override | ||
public void log(ChannelLogLevel level, String messageFormat, Object... args) { | ||
} | ||
}; | ||
|
||
private final int defaultPort; | ||
private final ProxyDetector proxyDetector; | ||
private final SynchronizationContext syncContext; | ||
private final ServiceConfigParser serviceConfigParser; | ||
private final ChannelLogger channelLogger; | ||
@Nullable private final Executor executor; | ||
|
||
Args( | ||
Integer defaultPort, | ||
ProxyDetector proxyDetector, | ||
SynchronizationContext syncContext, | ||
ServiceConfigParser serviceConfigParser, | ||
@Nullable ChannelLogger channelLogger, | ||
@Nullable Executor executor) { | ||
this.defaultPort = checkNotNull(defaultPort, "defaultPort not set"); | ||
this.proxyDetector = checkNotNull(proxyDetector, "proxyDetector not set"); | ||
this.syncContext = checkNotNull(syncContext, "syncContext not set"); | ||
this.serviceConfigParser = checkNotNull(serviceConfigParser, "serviceConfigParser not set"); | ||
if (channelLogger == null) { | ||
this.channelLogger = NOOP_CHANNEL_LOGGER; | ||
} else { | ||
this.channelLogger = channelLogger; | ||
} | ||
this.executor = executor; | ||
} | ||
|
||
|
@@ -466,6 +483,15 @@ public ServiceConfigParser getServiceConfigParser() { | |
return serviceConfigParser; | ||
} | ||
|
||
/** | ||
* Returns the {@link ChannelLogger} for the Channel served by this NameResolver. | ||
* | ||
* @since 1.26.0 | ||
*/ | ||
public ChannelLogger getChannelLogger() { | ||
return channelLogger; | ||
} | ||
|
||
/** | ||
* Returns the Executor on which this resolver should execute long-running or I/O bound work. | ||
* Null if no Executor was set. | ||
|
@@ -485,6 +511,7 @@ public String toString() { | |
.add("proxyDetector", proxyDetector) | ||
.add("syncContext", syncContext) | ||
.add("serviceConfigParser", serviceConfigParser) | ||
.add("channelLogger", channelLogger) | ||
.add("executor", executor) | ||
.toString(); | ||
} | ||
|
@@ -500,6 +527,7 @@ public Builder toBuilder() { | |
builder.setProxyDetector(proxyDetector); | ||
builder.setSynchronizationContext(syncContext); | ||
builder.setServiceConfigParser(serviceConfigParser); | ||
builder.setChannelLogger(channelLogger); | ||
builder.setOffloadExecutor(executor); | ||
return builder; | ||
} | ||
|
@@ -523,6 +551,7 @@ public static final class Builder { | |
private ProxyDetector proxyDetector; | ||
private SynchronizationContext syncContext; | ||
private ServiceConfigParser serviceConfigParser; | ||
private ChannelLogger channelLogger; | ||
private Executor executor; | ||
|
||
Builder() { | ||
|
@@ -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 commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed offline, we are trying to make a "soft requirement" for We do not call out if it is required or optional in builder. Implementors that care about this field know what to do. |
||
* | ||
* @since 1.26.0 | ||
*/ | ||
public Builder setChannelLogger(ChannelLogger channelLogger) { | ||
this.channelLogger = checkNotNull(channelLogger); | ||
return this; | ||
} | ||
|
||
/** | ||
* See {@link Args#getOffloadExecutor}. This is an optional field. | ||
* | ||
|
@@ -585,7 +624,10 @@ public Builder setOffloadExecutor(Executor executor) { | |
* @since 1.21.0 | ||
*/ | ||
public Args build() { | ||
return new Args(defaultPort, proxyDetector, syncContext, serviceConfigParser, executor); | ||
return | ||
new Args( | ||
defaultPort, proxyDetector, syncContext, serviceConfigParser, | ||
channelLogger, executor); | ||
} | ||
} | ||
} | ||
|
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).