-
Notifications
You must be signed in to change notification settings - Fork 2.4k
[Repository S3] Move async http client to CRT from Netty and add configurability to chose client via repo settings #18800
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
[Repository S3] Move async http client to CRT from Netty and add configurability to chose client via repo settings #18800
Conversation
❌ Gradle check result for 21d84e2: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for 838c875: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
@pranikum Just to clarify, we’re not adding the S3 CRT client in addition to the existing S3 async client, but rather giving an option to replace the Netty HTTP client used in the existing S3 async client with the CRT HTTP client.
Therefore, with this change, we can't say that we are resolving the above issue, which explicitly calls to explore S3 based CRT client. (see ref) |
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java
Outdated
Show resolved
Hide resolved
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Repository.java
Outdated
Show resolved
Hide resolved
Not sure if I understood this. We are making "crt" as the default based on setting. The value can be overridden based on setting to "netty" |
Got the comment. Thanks for pointing the same. Updating the Issue/PR description |
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3AsyncService.java
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #18800 +/- ##
============================================
+ Coverage 72.80% 72.87% +0.07%
+ Complexity 69399 69394 -5
============================================
Files 5648 5648
Lines 319230 319329 +99
Branches 46174 46194 +20
============================================
+ Hits 232421 232723 +302
+ Misses 68003 67690 -313
- Partials 18806 18916 +110 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c1b205e
to
149a42b
Compare
❌ Gradle check result for 149a42b: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall .
.../repository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerRetriesTests.java
Outdated
Show resolved
Hide resolved
❌ Gradle check result for e1347ad: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for f75436d: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
.../src/internalClusterTest/java/org/opensearch/repositories/s3/S3BlobStoreRepositoryTests.java
Show resolved
Hide resolved
...src/internalClusterTest/java/org/opensearch/repositories/s3/S3RepositoryThirdPartyTests.java
Show resolved
Hide resolved
plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3AsyncService.java
Outdated
Show resolved
Hide resolved
❌ Gradle check result for f75436d: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
…le registerd repositories. Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
e9fb30b
to
b4a21c5
Compare
❌ Gradle check result for b4a21c5: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Pranit Kumar <[email protected]>
❕ Gradle check result for 799c5f6: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
…igurability to chose client via repo settings (opensearch-project#18800) Signed-off-by: Pranit Kumar <[email protected]>
…igurability to chose client via repo settings (opensearch-project#18800) Signed-off-by: Pranit Kumar <[email protected]>
…igurability to chose client via repo settings (opensearch-project#18800) Signed-off-by: Pranit Kumar <[email protected]>
…igurability to chose client via repo settings (opensearch-project#18800) Signed-off-by: Pranit Kumar <[email protected]>
Description
As part of this PR we are adding support for
AwsCrtAsyncHttpClient
in theS3AsyncService
. Currently the default Http client used was NettyAsyncHttpClient. However as per our perf testing we have seen an improvement of approximately 5-7% when usingAwsCrtAsyncHttpClient
instead ofNettyAsyncHttpClient
. We will be makingAwsCrtAsyncHttpClient
as default.NettyAsyncHttpClient
will still be supported through settingRelated Issues
Resolves #18535
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.