Propagate Disable User Agent Config to Http Client

[ankitsultana]

In #10895 I introduced this config, but I had made an error. I never actually set this config in the final Apache HttpClient that gets created. This fixes that.

Test Plan: Several integration/unit-tests that run as part of the PR use the HttpClient so that should take care of smoke testing.

Side Note: The reason we had to do this was because we were running into a JDK bug we reported in #10894. But I think a better way to avoid this issue to set the http.agent system property, since the commons http-client does this:

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 61.73%. Comparing base (6b0cfeb) to head (56d8697).
Report is 1 commits behind head on master.

Files	Patch %	Lines
...org/apache/pinot/common/utils/http/HttpClient.java	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #12479       +/-   ##
=============================================
+ Coverage     46.87%   61.73%   +14.86%     
+ Complexity      948      207      -741     
=============================================
  Files          1829     2436      +607     
  Lines         96638   133220    +36582     
  Branches      15656    20636     +4980     
=============================================
+ Hits          45300    82247    +36947     
+ Misses        48104    44929     -3175     
- Partials       3234     6044     +2810     

Flag	Coverage Δ
custom-integration1	<0.01% <0.00%> (?)
integration	<0.01% <0.00%> (?)
integration1	<0.01% <0.00%> (?)
integration2	0.00% <0.00%> (?)
java-11	61.68% <0.00%> (+14.87%)	⬆️
java-21	61.62% <0.00%> (+14.90%)	⬆️
skip-bytebuffers-false	61.71% <0.00%> (+14.85%)	⬆️
skip-bytebuffers-true	61.58% <0.00%> (+14.92%)	⬆️
temurin	61.73% <0.00%> (+14.86%)	⬆️
unittests	61.73% <0.00%> (+14.85%)	⬆️
unittests1	46.88% <0.00%> (+<0.01%)	⬆️
unittests2	27.72% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[satishd]

Thanks @ankitsultana for the PR.

Is this change covered with any of the existing UTs? If so, it should have been failed earlier and it should pass with this change.

[ankitsultana]

I am not sure how to test this via UTs. The user-agent config is passed to the Apache Commons HttpClient.

After the client is created, there's no way to verify what the user-agent config value is. You can refer to org.apache.http.impl.client.HttpClientBuilder#build. The part where this config is used is in the screenshot in the description.

Existing UTs take care of sanity testing the change.

[satishd]

We can extract a method like below and add a UT for this method to check whether the HttpClientBuilder instance is updated with all the targeted configs passed to HttpClientConfig while building HttpClient. WDYT?

  static void setConfigsToHttpClientBuilder(HttpClientConfig httpClientConfig, HttpClientBuilder httpClientBuilder) {
    if (httpClientConfig.getMaxConnTotal() > 0) {
      httpClientBuilder.setMaxConnTotal(httpClientConfig.getMaxConnTotal());
    }
    if (httpClientConfig.getMaxConnPerRoute() > 0) {
      httpClientBuilder.setMaxConnPerRoute(httpClientConfig.getMaxConnPerRoute());
    }
    if (httpClientConfig.isDisableDefaultUserAgent()) {
      httpClientBuilder.disableDefaultUserAgent();
    }
  }

[Jackie-Jiang]

Since the fix is very straight forward, I'll merge it now. We can consider adding a test separately

[satishd]

Created a followup issue #12519