Use singleton instance creation for HttpClientProviders #3
Conversation
| DEFAULT_HTTP_CLIENT.compareAndSet(null, new JdkAsyncHttpClientBuilder().build()); | ||
|
|
||
| return DEFAULT_HTTP_CLIENT.get(); | ||
| return new JdkAsyncHttpClientBuilder().build(); |
There was a problem hiding this comment.
Is it possible for users to pass configurations when creating a JdkAsyncHttpClient ?
| /** | ||
| * An {@link HttpClientProvider} that provides an implementation of HttpClient based on native JDK HttpClient. | ||
| * <p> | ||
| * NOTE: This implementation is only available in Java 11+ as that is when {@link java.net.http.HttpClient} was | ||
| * introduced. | ||
| */ | ||
| public final class JdkHttpClientProvider implements HttpClientProvider { | ||
| private static final AtomicReference<HttpClient> DEFAULT_HTTP_CLIENT = new AtomicReference<>(); |
There was a problem hiding this comment.
I was thinking that we should not use shared HTTP clients by default as it will break any existing users using this functionality.
Since they may have dependencies on the default client and must be assuming that each SDK creates a new instance of the client hence relying on the per-client configurations (connection pool size, thread counts etc)
| /** | ||
| * Flag to disable sharing of http clients if not configured by the user. | ||
| */ | ||
| public static final String PROPERTY_HTTP_CLIENT_SINGLETON_DISABLED = "PROPERTY_HTTP_CLIENT_SINGLETON_DISABLED"; |
There was a problem hiding this comment.
Thoughts on flipping this configuration setting to be AZURE_HTTP_CLIENT_SINGLETON_ENABLED as the default would match what the current pattern is in the SDKs, not using a singleton.
| if (clientOptions.getConfiguration() != null) { | ||
| DEFAULT_HTTP_CLIENT.compareAndSet(null, new OkHttpAsyncHttpClientBuilder().build()); | ||
| return DEFAULT_HTTP_CLIENT.get(); | ||
| } |
There was a problem hiding this comment.
Shouldn't this code block only be reached if clientOptions is null?
| HttpClient httpClient = new NettyAsyncHttpClientBuilder().build(); | ||
| DEFAULT_HTTP_CLIENT.compareAndSet(null, httpClient); | ||
| // by default use a singleton http client | ||
| return httpClient; |
There was a problem hiding this comment.
A little confused here as this isn't using a singleton, right?
samvaity
changed the title
* health insights sdk for java * rename country to countryOrRegion * update product name to azure-health-insights * update cspell with missing words * update root pom.xml and version_client.txt * update dependency versions * update dependency versions (#2) * skip jacoco * fix spell checks * Asaflevi/feature/healthinsights sdk for java (#3) * update dependency versions * skip jacoco * fix spell checks * skip jacoco * linting * remove redundant plugins * remove test.yaml files * remove redundant package.json file * fix sampes and readmes * spell * fix linting * fix sample repository reference * remove duplicate dependency (mockito) * alignment (readme) * fix readme title * setPlaybackSyncPollerPollInterval * update serviceversion class (emitter 0.5.1) * remove impressions png
Updating https://github.com/Azure/azure-sdk-for-java/pull/23123/files PR to use a singleton instance creation for HTTP clients.
This helps the customers against a misused coding pattern.
This PR adds test cases.