Skip to content

Merge Apache 5.x Preview to Master #6220

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

Open
wants to merge 35 commits into
base: master
Choose a base branch
from
Open

Conversation

joviegas
Copy link
Contributor

Motivation and Context

  • Create a new client package with Apache 5.x

Modifications

  • New apckage added

Testing

  • Junits added

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

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

joviegas and others added 30 commits April 30, 2025 08:27
* Add initial module for Apache5x for seting up package

* Add based on new module checklist
…pache5SDKHttpClient (#6088)

* Add initial module for Apache5x for seting up package

* Add based on new module checklist

* Baseline all the classes from Apache4 SDK client to the new Apache5 module
…aring Checkstyles and spotbug issues (#6100)

* Phase 2 , getting Apache 5 compilation and Junit ready along with clearing Checkstyles and spotbug issues

* Handle comments from review

* Handle comments from Zoe
…reamRequestEntity repeatability (#6132)

* Fix HTTP authentication retry failures by improving RepeatableInputStreamRequestEntity repeatability

* Upated test cases

* Handled comments
* Fix architecture test failures for apache5.x

* Checkstyle issues
…ientConnectionManager for Connection Manager (#6147)

* Fix architecture test failures for apache5.x

* Checkstyle issues

* Update to use PoolingHttpClientConnectionManager class reference that is implementation of HttpClientConnectionManager
…6154)

* Fix architecture test failures for apache5.x

* Checkstyle issues

* Update to use PoolingHttpClientConnectionManager class reference that is implementation of HttpClientConnectionManager

* Fix stream reset failure in RepeatableInputStreamRequestEntity by storing content reference to avoid multiple ContentStreamProvider.newStream() calls that cause IOException when retrying requests with non-resettable streams

* writeTo_ConcurrentWrites_HandlesCorrectly no longer needed since even Apache 4.x doesnot suports this
…zil.json (#6191)

* Fix architecture test failures for apache5.x

* Checkstyle issues

* Update to use PoolingHttpClientConnectionManager class reference that is implementation of HttpClientConnectionManager

* Fix stream reset failure in RepeatableInputStreamRequestEntity by storing content reference to avoid multiple ContentStreamProvider.newStream() calls that cause IOException when retrying requests with non-resettable streams

* writeTo_ConcurrentWrites_HandlesCorrectly no longer needed since even Apache 4.x doesnot suports this

* Fix connectionPoolingWorks by setting  skipping setConnectionTimeToLive is value is set to 0 since 0 is treated as Infinite timeToLive in Sdk and Apache 4.x but treated as immediate closeConnection in apache 5.x

* disableAutomaticRetries in Apache 5.x since SDK handles retries , also define Apache5 dependencies in .brazil.json

* Added Test case for Async , handled review ocmments
* Fix architecture test failures for apache5.x

* Checkstyle issues

* Update to use PoolingHttpClientConnectionManager class reference that is implementation of HttpClientConnectionManager

* Fix stream reset failure in RepeatableInputStreamRequestEntity by storing content reference to avoid multiple ContentStreamProvider.newStream() calls that cause IOException when retrying requests with non-resettable streams

* writeTo_ConcurrentWrites_HandlesCorrectly no longer needed since even Apache 4.x doesnot suports this

* Fix connectionPoolingWorks by setting  skipping setConnectionTimeToLive is value is set to 0 since 0 is treated as Infinite timeToLive in Sdk and Apache 4.x but treated as immediate closeConnection in apache 5.x

* disableAutomaticRetries in Apache 5.x since SDK handles retries , also define Apache5 dependencies in .brazil.json

* Added Test case for Async , handled review ocmments

* Donot do buffer the response using BufferedHttpEntity since it might cause memory issue, this behaviour is same as Apache4.x

* Fix compilation issues

* Fix checkstyle  issues

* Remove test which are specific to apache http
* Fix architecture test failures for apache5.x

* Checkstyle issues

* Update to use PoolingHttpClientConnectionManager class reference that is implementation of HttpClientConnectionManager

* Fix stream reset failure in RepeatableInputStreamRequestEntity by storing content reference to avoid multiple ContentStreamProvider.newStream() calls that cause IOException when retrying requests with non-resettable streams

* writeTo_ConcurrentWrites_HandlesCorrectly no longer needed since even Apache 4.x doesnot suports this

* Fix connectionPoolingWorks by setting  skipping setConnectionTimeToLive is value is set to 0 since 0 is treated as Infinite timeToLive in Sdk and Apache 4.x but treated as immediate closeConnection in apache 5.x

* disableAutomaticRetries in Apache 5.x since SDK handles retries , also define Apache5 dependencies in .brazil.json

* Added Test case for Async , handled review ocmments

* Donot do buffer the response using BufferedHttpEntity since it might cause memory issue, this behaviour is same as Apache4.x

* Fix compilation issues

* Fix checkstyle  issues

* Remove test which are specific to apache http

* Add benchmark for Apache5 and add Streaming Api test cases
… alternatives (#6211)

* Clean up unused APIs and add test to make sure it can be handled with alternatives

* Added NTCredentials to keep backward compatibilty with Apache4.x
… version (#6214)

* Fix architecture test failures for apache5.x

* Checkstyle issues

* Update to use PoolingHttpClientConnectionManager class reference that is implementation of HttpClientConnectionManager

* Fix stream reset failure in RepeatableInputStreamRequestEntity by storing content reference to avoid multiple ContentStreamProvider.newStream() calls that cause IOException when retrying requests with non-resettable streams

* writeTo_ConcurrentWrites_HandlesCorrectly no longer needed since even Apache 4.x doesnot suports this

* Fix connectionPoolingWorks by setting  skipping setConnectionTimeToLive is value is set to 0 since 0 is treated as Infinite timeToLive in Sdk and Apache 4.x but treated as immediate closeConnection in apache 5.x

* disableAutomaticRetries in Apache 5.x since SDK handles retries , also define Apache5 dependencies in .brazil.json

* Added Test case for Async , handled review ocmments

* Donot do buffer the response using BufferedHttpEntity since it might cause memory issue, this behaviour is same as Apache4.x

* Fix compilation issues

* Fix checkstyle  issues

* Remove test which are specific to apache http

* Add benchmark for Apache5 and add Streaming Api test cases

* Update Apache5 to 5.5
joviegas and others added 4 commits June 30, 2025 09:23
* Fix architecture test failures for apache5.x

* Checkstyle issues

* Update to use PoolingHttpClientConnectionManager class reference that is implementation of HttpClientConnectionManager

* Fix stream reset failure in RepeatableInputStreamRequestEntity by storing content reference to avoid multiple ContentStreamProvider.newStream() calls that cause IOException when retrying requests with non-resettable streams

* writeTo_ConcurrentWrites_HandlesCorrectly no longer needed since even Apache 4.x doesnot suports this

* Fix connectionPoolingWorks by setting  skipping setConnectionTimeToLive is value is set to 0 since 0 is treated as Infinite timeToLive in Sdk and Apache 4.x but treated as immediate closeConnection in apache 5.x

* disableAutomaticRetries in Apache 5.x since SDK handles retries , also define Apache5 dependencies in .brazil.json

* Added Test case for Async , handled review ocmments

* Donot do buffer the response using BufferedHttpEntity since it might cause memory issue, this behaviour is same as Apache4.x

* Fix compilation issues

* Fix checkstyle  issues

* Remove test which are specific to apache http

* Add benchmark for Apache5 and add Streaming Api test cases

* Update Apache5 to 5.5

* Preview ready , addressing open TODOs

* Added PublicApi since checkstyle was failing
@joviegas joviegas requested a review from a team as a code owner June 30, 2025 21:07
try {
SSLContext sslcontext = SSLContext.getInstance("TLS");
// http://download.java.net/jdk9/docs/technotes/guides/security/jsse/JSSERefGuide.html
sslcontext.init(keyManagers, trustManagers, null);

Check failure

Code scanning / CodeQL

`TrustManager` that accepts all certificates High

This uses
TrustManager
, which is defined in
Apache5HttpClient$ApacheConnectionManagerFactory$
and trusts any certificate.
<dependency>
<groupId>org.apache.httpcomponents.client5</groupId>
<artifactId>httpclient5</artifactId>
<version>5.5</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we define this in bom-internal so that it's easier to manage?

<dependency>
<groupId>org.apache.httpcomponents.core5</groupId>
<artifactId>httpcore5</artifactId>
<version>5.3.4</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Comment on lines +106 to +109
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest-all</artifactId>
<scope>test</scope>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason we are using hamcrest? assertJ is preferred

@SdkPublicApi
public final class Apache5HttpClient implements SdkHttpClient {

public static final String CLIENT_NAME = "Apache5";
Copy link
Contributor

Choose a reason for hiding this comment

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

private?


public PoolingHttpClientConnectionManager create(Apache5HttpClient.DefaultBuilder configuration,
AttributeMap standardOptions) {
// TODO : Deprecated method needs to be removed with new replacements
Copy link
Contributor

Choose a reason for hiding this comment

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

Any plans to address this?

Duration connectionTtl = standardOptions.get(SdkHttpConfigurationOption.CONNECTION_TIME_TO_LIVE);
if (!connectionTtl.isZero()) {
// Skip TTL=0 to maintain backward compatibility (infinite in 4.x vs immediate expiration in 5.x)
builder.setConnectionTimeToLive(TimeValue.of(connectionTtl.toMillis(), TimeUnit.MILLISECONDS));
Copy link
Contributor

Choose a reason for hiding this comment

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

setConnectionTimeToLive also seems to be deprecated, should we use setDefaultConnectionConfig instead?

Comment on lines +713 to +715
cm.setDefaultMaxPerRoute(standardOptions.get(SdkHttpConfigurationOption.MAX_CONNECTIONS));
cm.setMaxTotal(standardOptions.get(SdkHttpConfigurationOption.MAX_CONNECTIONS));
cm.setDefaultSocketConfig(buildSocketConfig(standardOptions));
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems a bit odd that we are configuring them after PoolingHttpClientConnectionManager has been built. Can we configure them before line 710?

return request.getUri();
}

private void addRequestConfig(HttpUriRequestBase base,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to disable uri normalization as we did in 4.x?

@@ -0,0 +1,43 @@
[
{
"name": "import software.amazon.awssdk.http.apache5.ApacheSdkHttpService",
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be updated

"name": "org.apache.http.client.config.RequestConfig$Builder",
"methods": [
{
"name": "setNormalizeUri"
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think setNormalizeUri is used

)

* Updated Version as -PREVIEW

* japi cmp needs to be disables since this is a new version and we dont have old version to compare
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants