Implement a HttpUrlConnection and Apache HC4 based channels #335
Conversation
carterkozak
changed the title
carterkozak
force-pushed
the
ckozak/blocking_channels
branch
from
e14fb78 to
723ead7
Compare
carterkozak
force-pushed
the
ckozak/blocking_channels
branch
from
723ead7 to
b718a73
Compare
.../src/main/java/com/palantir/dialogue/httpurlconnection/HttpUrlConnectionBlockingChannel.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/palantir/dialogue/httpurlconnection/HttpUrlConnectionBlockingChannel.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/palantir/dialogue/httpurlconnection/HttpUrlConnectionBlockingChannel.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/palantir/dialogue/httpurlconnection/HttpUrlConnectionBlockingChannel.java
Outdated
Show resolved
Hide resolved
...ent/src/test/java/com/palantir/dialogue/httpurlconnection/HttpUrlConnectionChannelsTest.java
Outdated
Show resolved
Hide resolved
...ent/src/test/java/com/palantir/dialogue/httpurlconnection/HttpUrlConnectionChannelsTest.java
Show resolved
Hide resolved
dialogue-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannel.java
Outdated
Show resolved
Hide resolved
dialogue-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannel.java
Outdated
Show resolved
Hide resolved
dialogue-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannel.java
Outdated
Show resolved
Hide resolved
dialogue-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannel.java
Outdated
Show resolved
Hide resolved
Generate changelog in
|
| final class ApacheHttpClientBlockingChannel implements BlockingChannel { | ||
| private static final Logger log = LoggerFactory.getLogger(ApacheHttpClientBlockingChannel.class); | ||
|
|
||
| private final CloseableHttpClient client; |
There was a problem hiding this comment.
Ok so at the moment we're potentially leaving a connection manager open by not closing this anywhere, which is why you wrote up #339 (comment) ?
There was a problem hiding this comment.
That's correct.
| } | ||
| } | ||
| headers = tmpHeaders; | ||
| } |
There was a problem hiding this comment.
seeing the lazy initialization of this makes me wonder if we should document the thread-safety expectations of Response somewhere?
| String value = header.getValue(); | ||
| if (value != null) { | ||
| tmpHeaders | ||
| .computeIfAbsent(header.getName(), _name -> new ArrayList<>(1)) |
There was a problem hiding this comment.
you sure you don't want to pull this out to a static method reference? ;) it's gonna allocate one whole lambda every time we call headers()
There was a problem hiding this comment.
Oh you know I want to. Keeping simple for now :-)
...ogue-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientBlockingChannel.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public boolean isChunked() { | ||
| return true; |
There was a problem hiding this comment.
Out of curiosity, are there ever worlds where we explicitly wouldn't want chunked? Do people just do it when you know the request size and want to save the bytes in between each chunk?
There was a problem hiding this comment.
That's correct. There's an optimization where we could internally buffer data, and if we hit the end of content before something like 8k or 16k bytes we could send a content-length request.
We already have relatively large buffers configured at the OS level so we're fine writing to the socket, we just can't walk back to the headers and add a content-length.
|
|
||
| private HttpUrlConnectionChannels() {} | ||
|
|
||
| public static Channel create(ClientConfiguration conf, UserAgent baseAgent, TaggedMetricRegistry metrics) { |
There was a problem hiding this comment.
Low importance as I think this will be used for benchmarking over prod usage, but might be worth doing the same ClientConf preconditions here?
Kinda spicy that we literally can't even control cipher suites...
There was a problem hiding this comment.
Perhaps in a follow-up, I have a WIP to move some of the validation to Channels.create we pick the node selection strategy, qos, etc.
I'll open a PR to update the readme with a description of each of our current clients and describe our confidence (e.g. don't use java9 or httpurlconnection)
| include 'dialogue-client-test-lib' | ||
| include 'dialogue-core' | ||
| include 'dialogue-example' | ||
| include 'dialogue-httpurlconnection-client' | ||
| include 'dialogue-hc4-client' | ||
| include 'dialogue-java-client' |
There was a problem hiding this comment.
I know it's not this PR, but could we rename this to dialogue-java9-client as the -java suffix is a bit overloaded in conjure-land
There was a problem hiding this comment.
We can do that separately. I think email-service will need to be updated.
settings.gradle
Outdated
| include 'dialogue-client-test-lib' | ||
| include 'dialogue-core' | ||
| include 'dialogue-example' | ||
| include 'dialogue-httpurlconnection-client' | ||
| include 'dialogue-hc4-client' |
There was a problem hiding this comment.
dialogue-apache-hc4? seems a tad over concise for anyone who doesn't know what HttpComponents4 are
There was a problem hiding this comment.
I thought hc stood for HttpClient, though that name is overloaded with java these days ;-)
Feign calls it httpclient https://github.com/OpenFeign/feign/tree/master/httpclient
As does dropwizard-metrics: https://metrics.dropwizard.io/3.1.0/manual/httpclient/
+1 for apache-hc4 to disambiguate from other clients.
Gives us a data point to compare other clients with.