Skip to content

Replace httpclient.execute call with httpclient.executeOpen as mentioned in deprecation notes of httpclient.execute #6298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

joviegas
Copy link
Contributor

@joviegas joviegas commented Jul 24, 2025

Motivation and Context

The Apache HttpClient 5 execute() method is deprecated with a warning to use executeOpen() instead when we need to keep the response object open after request execution. Our Apache5HttpClient implementation requires keeping the response open because we wrap it in an AbortableInputStream for proper resource management.

This change eliminates the deprecation warning and ensures we're using the recommended API for our use case.

Modifications

  1. Apache5HttpClient.java:

    • Replaced deprecated httpClient.execute() with httpClient.executeOpen()
    • Added determineTarget() method that uses RoutingSupport.determineHost() to extract the target host from the request
    • Added proper exception handling to convert HttpException to ClientProtocolException
    • Added necessary imports for RoutingSupport and ClientProtocolException
  2. Apache5SdkHttpClient.java:

    • Added executeOpen() method to the delegate implementation to support the new API
  3. MetricReportingTest.java:

    • Updated mock setup to use executeOpen() instead of deprecated execute()
    • Cleaned up unused imports
    • Minor formatting improvements

Testing

  • All existing unit tests pass without modification
  • Updated MetricReportingTest to mock the new executeOpen() method
  • Verified that the response handling and AbortableInputStream wrapping continue to work correctly
  • No functional changes to the HTTP client behavior - this is purely an API migration

License

  • I confirm that this pull request can be released under the Apache 2 license

@joviegas joviegas requested a review from a team as a code owner July 24, 2025 23:31
@joviegas joviegas force-pushed the joviegas/apache5_replace_DeprecatedAPIs_2 branch from 651f569 to d944a98 Compare July 24, 2025 23:35
@joviegas joviegas force-pushed the joviegas/apache5_replace_DeprecatedAPIs_2 branch from d944a98 to e734ebc Compare July 24, 2025 23:37
@joviegas joviegas force-pushed the joviegas/apache5_replace_DeprecatedAPIs_2 branch from 2af2770 to daf8d7e Compare July 25, 2025 14:43
Copy link

Copy link
Contributor

@Fred1155 Fred1155 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also add an test to check the thrown exception since we extracted the logic outside but that's optional

@joviegas
Copy link
Contributor Author

We can also add an test to check the thrown exception since we extracted the logic outside but that's optional

I investigated adding test cases to verify the exception handling in determineTarget(), but it's not feasible to trigger this code path in practice. The SDK performs validation at the request builder level and in higher-level components, ensuring that only valid targets reach the low-level HTTP client. Any request that would cause RoutingSupport.determineHost() to throw an HttpException is already rejected by the SDK's validation layer before reaching the Apache client.

@joviegas joviegas merged commit e88d20d into feature/master/apache5x Jul 25, 2025
28 checks passed
Copy link

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants