Introduce HttpProviders and GrpcProviders
#2137
Conversation
|
|
||
| @Override | ||
| public GrpcClientBuilder<U, R> defaultTimeout(final Duration defaultTimeout) { | ||
| delegate = delegate.defaultTimeout(defaultTimeout); |
There was a problem hiding this comment.
When I had all builder methods as
@Override
public GrpcClientBuilder<U, R> defaultTimeout(final Duration defaultTimeout) {
delegate.defaultTimeout(defaultTimeout);
return this;
}Spotbugs complained that the return value is discarded. I though to suppress it because all our builder implementations always return this. But then considered a use-case when users may have an immutable provider or builder impl that always return a new instance. LMK what you think. Should we support that and reassign the delegate or is it better to keep delegate as final?
There was a problem hiding this comment.
I don't see any alternative to assigning delegate with each delegate invocation. Eventually someone will get clever and implement a builder which doesn't return this and we will have to fix it anyway. I've had to fix very non-obvious problems more than once where failing to reassign the builder at the end of the chain meant that the operations in the chain were lost. It is always a frustrating bug to locate.
Motivation: Allow users intercept static factories that create client/server builders to enhance builders behavior. Users can use these providers to override default values on the builders or to intercept builders for metrics purposes. Modifications: - Add `HttpProviders` and `GrpcProviders` that define provider interfaces for `ServiceLoader`; - Test how users can intercept builders; Result: Users can change the default builders using providers that are automatically loaded via `ServiceLoader`.
idelpivnitskiy
force-pushed
the
service-loader
branch
from
b81b2da to
ac1530d
Compare
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcProviders.java
Show resolved
Hide resolved
| /** | ||
| * A factory to create <a href="https://www.grpc.io">gRPC</a> clients. | ||
| */ | ||
| public final class GrpcClients { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(GrpcClients.class); | ||
|
|
||
| private static final List<GrpcClientBuilderProvider> PROVIDERS; |
There was a problem hiding this comment.
I wonder if putting these in the GrpcProviders class and exposing a static method wouldn't also satisfy the desire for discovering what providers are installed which some users will probably want to know..
There was a problem hiding this comment.
Let's see if logging is enough for now and defer expanding the public API surface
servicetalk-utils-internal/src/main/java/io/servicetalk/utils/internal/ServiceLoaderUtils.java
Outdated
Show resolved
Hide resolved
| * @param <T> type of the provider | ||
| * @return a list of loaded providers for the specified class | ||
| */ | ||
| public static <T> List<T> loadProviders(final Class<T> clazz, final ClassLoader classLoader, final Logger logger) { |
There was a problem hiding this comment.
Perhaps returning a Set or Collection since we don't actually support ordering and there should be no duplicate entries. We could make it an NavigableSet later if we decided to support ordering mechanism.
There was a problem hiding this comment.
Discussed offline, posting here for visibility:
This is an internal utility.
ServiceLoader implements Iterable. Will be more predictable to follow the result of the ServiceLoader, including the order. If it decides to load multiple equal instances (not sure if this is possible), we should preserve this behavior. Also, there are no requirements for providers to implement equals/hashCode.
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcProviders.java
Show resolved
Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpClients.java
Show resolved
Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpClients.java
Show resolved
Hide resolved
| import java.net.InetSocketAddress; | ||
| import java.net.SocketAddress; | ||
| import java.util.List; | ||
|
|
||
| import static io.servicetalk.utils.internal.ServiceLoaderUtils.loadProviders; | ||
|
|
||
| /** | ||
| * Factory methods for building HTTP Servers backed by {@link ServerContext}. |
There was a problem hiding this comment.
The connection to ServerContext is not clear from reading this class.
There was a problem hiding this comment.
Updated to make consistent with other factories.
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpServers.java
Show resolved
Hide resolved
servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpProvidersTest.java
Show resolved
Hide resolved
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcProviders.java
Show resolved
Hide resolved
|
By some reason, comments were posted twice. I deleted duplicated comments |
Motivation:
Allow users to intercept static factories that create client/server
builders to enhance builders' behavior. Users can use these providers
to override default values on the builders or to intercept builders for
metrics purposes.
Modifications:
HttpProvidersandGrpcProvidersthat define providerinterfaces for
ServiceLoader;Result:
Users can change the default builders using providers that are
automatically loaded via
ServiceLoader.