Conversation
Due to historic reasons, this class was placed in the xml package. As it evolved into a full block generic HTTP client, we moved it to the commons package.
By trying to re-use existing HttpClients, we can utilize the internal connection pool and thus drastically speed up repeated requests against the same host. As a client might be customized through its builder, we now ask for a clientSelector to distinguish these. If null is used as clientSelector, no re-use will happen.
andyHa
added
💣 BREAKING CHANGE
andyHa
requested review from
idlira,
jakobvogel,
jmuscireum,
mkeckmkeck and
sabieber
| * @return the underlying {@link HttpClient.Builder} | ||
| */ | ||
| public HttpClient.Builder modifyClient() { | ||
| return modifyClient(null); |
There was a problem hiding this comment.
Vielleicht wäre es eleganter den clientSelector einmalig im Constructor zu setzen anstatt ihn immer mitschleifen zu müssen?
There was a problem hiding this comment.
The selector is only required if the builder is customized. For many many cases, the single central http client can be re-used. Therefore, the constructor was left untouched and the selector has been moved here.
The few cases where we customize the builder are always either: modify timeout, provide a cookie manager or accept self-signed certificates.
Especially with cookie managers we want to enforce a decision if the resulting client can be cached or not and therefore require a parameter here.
| * @return the underlying {@link HttpClient.Builder} | ||
| * @deprecated Use {@link #modifyClient(String)} | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
Whats the rationale behind deprecating the methods without the clientSelector? Often we only need one request, and with simple ones like
Outcall call = new Outcall(URI.create(SERVER_URL));
String response = call.postData(params, StandardCharsets.ISO_8859_1).getData();no clientSelector needs to be specified. If however we want to add a cookie manager or want to trust self-signed certificates, suddenly we need to specify null. Either it should be always mandatory, so maybe we should add the clientSelector to the constructor, or it should be optional, maybe even with a new withClientSelector method.
There was a problem hiding this comment.
no, for these simple cases, no client selector is needed and thus none needs to be specified. If the cookie manager is specified, it is essential that a decision is made if the client (including the cookie manager) may be shared and re-used or not. Therefore, in this exact case, we require the parameter to enforce this decision to be made.
We now always prefer Duration in favor of ints. Also the documentation was fixes as it was plain wrong.
This is breaking as some constants were public. Use HttpHeaders from guava for proper constants.
7 participants
Description
Provides some cleanup and refactoring for the Outcall. The breaking change is that it
as been moved from the xml package to the commons package.
Additionally, some methods have been deprecated. Most notably, for all methods which customized the HttpClient.Builder via modifyClient (directly or indirectly) we
now ask for a clientSelector. If a client has already been built for a given clientSelector,
we re-use the existing client and thus support keepalive and connection pooling. This
should drastically speed up repeated requests to the same host.
If no re-use is intended, null can be used as selector.
Additional Notes
Checklist