ApacheHttpChannels exposes a Closeable instance #430
Conversation
This reverts commit 1e40d27.
Generate changelog in
|
| return BlockingChannelAdapter.of(new ApacheHttpClientBlockingChannel(client, url(uri))); | ||
| } | ||
|
|
||
| public static CloseableHttpClient createCloseableHttpClient(ClientConfiguration conf) { |
There was a problem hiding this comment.
A little bit wonky in public api since it's not clear what functionality the returned client provides compared to our channel wrappers. Will we cause bugs if we move functionality from the client into a channel?
At the same time, this should be an implementation dependency of components which simply provide channels, so it's probably fine.
There was a problem hiding this comment.
so I wanted to just convey the idea of some closeable shared resource - would you prefer if I wrap it in some opaque type so that people can't use it directly?
| } | ||
|
|
||
| public static Channel createSingleUri(String uri, CloseableHttpClient client) { | ||
| return BlockingChannelAdapter.of(new ApacheHttpClientBlockingChannel(client, url(uri))); |
There was a problem hiding this comment.
Another possibility is that we create a new client for each URI for additional isolation and avoiding contention in the connection pool.
There was a problem hiding this comment.
This API gives us the freedom to do that from our WC facade, because we can choose to pass in the same closeablehttpclient or not.
| @@ -75,15 +75,26 @@ | |||
|
|
|||
| private ApacheHttpClientChannels() {} | |||
|
|
|||
| public static Channel create(ClientConfiguration conf, UserAgent baseAgent) { | |||
| public static Channel create(ClientConfiguration conf, UserAgent agent) { | |||
There was a problem hiding this comment.
Should all channel implementations be closeable?
There was a problem hiding this comment.
I actually started with this, but I think it will get hella confusing in the scenario where our NodeSelectionStrategy channels contain a bunch of internal channels and possibly want to live reload a new uri... I think it's clearer to only make things closeable that actually directly hold resources, and if they don't hold resources then they can just be thrown away.
There was a problem hiding this comment.
I think queuedchannel is the only other piece that holds resources
|
👍 |
|
Released 0.10.1 |
| return BlockingChannelAdapter.of(new ApacheHttpClientBlockingChannel(client.client, url(uri))); | ||
| } | ||
|
|
||
| public static CloseableClient createCloseableHttpClient(ClientConfiguration conf) { |
There was a problem hiding this comment.
ClientCloseable? :) palantir/conjure-java#790
| return BlockingChannelAdapter.of(new ApacheHttpClientBlockingChannel(client.client, url(uri))); | ||
| } | ||
|
|
||
| public static CloseableClient createCloseableHttpClient(ClientConfiguration conf) { |
There was a problem hiding this comment.
ClientCloseable? :) palantir/conjure-java#790
|
|
||
| @Override | ||
| public String toString() { | ||
| return "SharedResource{client=" + client + '}'; |
| } | ||
|
|
||
| /** Intentionally opaque wrapper type - we don't want people using the inner Apache client directly. */ | ||
| public static final class CloseableClient implements Closeable { |
There was a problem hiding this comment.
why is this public if it's never used outside this class?
There was a problem hiding this comment.
It needs to be public because we create some of these in Witchcraft and then re-use them to avoid re-creating clients on every url live-reload.
See https://internal-github/foundry/witchcraft/pull/2233/files#diff-2819734323a961448d10003f575f1b4bR217
Before this PR
Previously, our
ApacheHttpChannels#createmethod would create a client pool internally, but gave us no way to shut this down.After this PR
==COMMIT_MSG==
ApacheHttpChannels exposes a Closeable instance, allowing callers to release resources when they determine the client is no longer needed.
==COMMIT_MSG==
Possible downsides?
clientPoolapproach which hid all these details.