chore: Migrate connectivity-destination-service to HttpClient5
#1028
Conversation
...ce/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceAdapter.java
Outdated
Show resolved
Hide resolved
newtork
left a comment
newtork
left a comment
There was a problem hiding this comment.
Please write a release note / compatibility note, indicating HttpClient 5 usage in destination retrieval. Users that overload the behavior for a workaround with HttpClientAccessor need to adopt and migrate to ApacheHttpClient5Accessor.
| ClassicHttpResponse response = | ||
| ResilienceDecorator.executeSupplier(tpDestinationVerifierSupplier, resilienceConfiguration) ) { | ||
| verifyTransparentProxyResponse(response, destinationName); | ||
| EntityUtils.consume(response.getEntity()); |
There was a problem hiding this comment.
(Major)
Please do not do this.
The response.getEntity() needs to be consumes (i.e. closed), otherwise the connection stays open for long time and reserves a slot of the connection pool. If too many "verifyTransparentProxyResponse" executions fail, the pool will be full and future runs will result in timeouts, because the pool is not releasing connections. These errors are super difficult to debug, when it comes to that.
Therefore, the finally block is required.
There was a problem hiding this comment.
It's a try-with-resources, it auto closes the response after the try. The entity consume night not even be necessary
There was a problem hiding this comment.
Interesting topic, I tried to understand what happens when you don't consume the entity before closing. Apparently, that will close the connection but may not give it back to connection pool(?)
We could test that by running like 500 HttpCient requests with closing but without consuming. If my concern is valid it will stop after 200 requests.
Since you are the one changing the code, you will test that :D
There was a problem hiding this comment.
500 requests each with 1 second delay, all in parallel
Total time for 500 requests: 10762 ms
500 failing requests each with 1 second delay, all in parallel
Total time for 500 failing requests: 2187 ms
There was a problem hiding this comment.
Furthermore, it works without the consume
There was a problem hiding this comment.
it works without the consume
I confirm. "consume" is already being called implicitly.
We could actually remove the explicit call here.
Context
SAP/ai-sdk-java-backlog#328.
HttpClient4 is very outdated
Feature scope:
Definition of Done
Documentation updatedRelease notes updated