Skip to content
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

Adding configurable connection count setting for S3 Sync Client #12028

Merged
merged 3 commits into from
Jan 29, 2024

Conversation

gbbafna
Copy link
Collaborator

@gbbafna gbbafna commented Jan 26, 2024

Description

  1. Add configurable S3 plugin's setting max_sync_connections for sync client's max connections.
  2. Uses existing setting connection_acquisition_timeout for sync client as well . Today it is only used in async client.

Related Issues

Resolves #12027

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

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.

@github-actions github-actions bot added the Storage:Durability Issues and PRs related to the durability framework label Jan 26, 2024
Copy link
Contributor

github-actions bot commented Jan 26, 2024

Compatibility status:

Checks if related components are compatible with change 63a50be

Incompatible components

Incompatible components: [https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/cross-cluster-replication.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/security.git]

Copy link
Contributor

❕ Gradle check result for 0140a0a: UNSTABLE

  • TEST FAILURES:
      2 org.opensearch.repositories.azure.AzureBlobStoreRepositoryTests.classMethod
      2 org.opensearch.remotestore.RemoteIndexPrimaryRelocationIT.testPrimaryRelocationWhileIndexing
      1 org.opensearch.repositories.azure.AzureBlobStoreRepositoryTests.testSnapshotAndRestore
      1 org.opensearch.index.IndexServiceTests.testAsyncTranslogTrimTaskOnClosedIndex

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (e017a9c) 71.36% compared to head (63a50be) 71.28%.
Report is 6 commits behind head on main.

Files Patch % Lines
...ing/allocation/allocator/RemoteShardsBalancer.java 83.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #12028      +/-   ##
============================================
- Coverage     71.36%   71.28%   -0.09%     
+ Complexity    59468    59397      -71     
============================================
  Files          4925     4925              
  Lines        279458   279475      +17     
  Branches      40631    40635       +4     
============================================
- Hits         199446   199223     -223     
- Misses        63437    63645     +208     
- Partials      16575    16607      +32     

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

Copy link
Collaborator

@Bukhtawar Bukhtawar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@deshsidd deshsidd left a comment

Choose a reason for hiding this comment

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

Code test coverage check not passing. Might need to add some more tests?

Copy link
Contributor

@deshsidd deshsidd left a comment

Choose a reason for hiding this comment

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

LGMT otherwise

Signed-off-by: Gaurav Bafna <[email protected]>
Copy link
Contributor

❕ Gradle check result for db45058: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.remotestore.RemoteIndexPrimaryRelocationIT.testPrimaryRelocationWhileIndexing

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Copy link
Contributor

❕ Gradle check result for 63a50be: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.repositories.azure.AzureBlobContainerRetriesTests.testReadNonexistentBlobThrowsNoSuchFileException

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@sachinpkale sachinpkale merged commit 4323af1 into opensearch-project:main Jan 29, 2024
30 checks passed
@sachinpkale sachinpkale added the backport 2.x Backport to 2.x branch label Jan 29, 2024
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-12028-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4323af1973c6c19b7ab35754f3ff1d6a5437dce9
# Push it to GitHub
git push --set-upstream origin backport/backport-12028-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-12028-to-2.x.

gbbafna added a commit to gbbafna/OpenSearch that referenced this pull request Jan 29, 2024
…search-project#12028)

* Adding configurable connection count setting for S3 Sync Client

Signed-off-by: Gaurav Bafna <[email protected]>

* Address PR Comments

Signed-off-by: Gaurav Bafna <[email protected]>

* Increase number of connections to 500

Signed-off-by: Gaurav Bafna <[email protected]>

---------

Signed-off-by: Gaurav Bafna <[email protected]>
sachinpkale pushed a commit that referenced this pull request Jan 29, 2024
…) (#12053)

* Adding configurable connection count setting for S3 Sync Client



* Address PR Comments



* Increase number of connections to 500



---------

Signed-off-by: Gaurav Bafna <[email protected]>
@dblock
Copy link
Member

dblock commented Feb 2, 2024

@gbbafna Why wasn't max_connections reused? What scenarios lead to needing two different settings?

@gbbafna
Copy link
Collaborator Author

gbbafna commented Feb 3, 2024

@gbbafna Why wasn't max_connections reused? What scenarios lead to needing two different settings?

We use sync client for get, list , delete and put for smaller sized files. Whereas async client is only used for multipart upload.

The sync client would always be used in create shard, which we want to give dedicated connections. Else , it might wait much longer for connection, causing a node drop. That's why we have introduced dedicated connections for now.

As we move everything to asyn client, we will need to address the cluster state applier thread prioritisation work as well.

peteralfonsi pushed a commit to peteralfonsi/OpenSearch that referenced this pull request Mar 1, 2024
…search-project#12028)

* Adding configurable connection count setting for S3 Sync Client

Signed-off-by: Gaurav Bafna <[email protected]>

* Address PR Comments

Signed-off-by: Gaurav Bafna <[email protected]>

* Increase number of connections to 500

Signed-off-by: Gaurav Bafna <[email protected]>

---------

Signed-off-by: Gaurav Bafna <[email protected]>
rayshrey pushed a commit to rayshrey/OpenSearch that referenced this pull request Mar 18, 2024
…search-project#12028)

* Adding configurable connection count setting for S3 Sync Client

Signed-off-by: Gaurav Bafna <[email protected]>

* Address PR Comments

Signed-off-by: Gaurav Bafna <[email protected]>

* Increase number of connections to 500

Signed-off-by: Gaurav Bafna <[email protected]>

---------

Signed-off-by: Gaurav Bafna <[email protected]>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…search-project#12028)

* Adding configurable connection count setting for S3 Sync Client

Signed-off-by: Gaurav Bafna <[email protected]>

* Address PR Comments

Signed-off-by: Gaurav Bafna <[email protected]>

* Increase number of connections to 500

Signed-off-by: Gaurav Bafna <[email protected]>

---------

Signed-off-by: Gaurav Bafna <[email protected]>
Signed-off-by: Shivansh Arora <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch backport-failed Storage:Durability Issues and PRs related to the durability framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[S3 Repository Plugin] Add configurable connection count setting for S3 Sync Client
7 participants