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

fileservice: limit the number of concurrent object storage operations to 100 #17330

Merged
merged 2 commits into from
Jul 7, 2024

Conversation

reusee
Copy link
Contributor

@reusee reusee commented Jul 4, 2024

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #17329

What this PR does / why we need it:

1000 is too high.

@reusee reusee requested a review from fengttt as a code owner July 4, 2024 10:46
@matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Jul 4, 2024
@matrix-meow matrix-meow added size/S Denotes a PR that changes [10,99] lines and removed size/XS Denotes a PR that changes [1, 9] lines labels Jul 5, 2024
@reusee reusee changed the title fileservice: set maxConnsPerHost to 100 fileservice: limit the number of object storage concurrent operations to 100 Jul 5, 2024
… to 100

fileservice: set maxConnsPerHost to 100

fileservice: fix http client in AwsSDKv2

fileservice: disable aws sdk client side rate limiter

fileservice: add objectStorageSemaphore
@reusee reusee changed the title fileservice: limit the number of object storage concurrent operations to 100 fileservice: limit the number of concurrent object storage operations to 100 Jul 5, 2024
@matrix-meow matrix-meow added size/M Denotes a PR that changes [100,499] lines and removed size/S Denotes a PR that changes [10,99] lines labels Jul 5, 2024
Copy link
Contributor

@fengttt fengttt left a comment

Choose a reason for hiding this comment

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

From the PR description I expect this is a one liner. What the other stuff is doing? Esp why we need a new imp of semaphore?

@reusee
Copy link
Contributor Author

reusee commented Jul 6, 2024

From the PR description I expect this is a one liner. What the other stuff is doing? Esp why we need a new imp of semaphore?

The MaxConnsPerHost option of the HTTP client doesn't limit the request concurrency, so a true semaphore for the object storage interface is required.
Some SDKs will use connection multiplexing protocols such as HTTP2, so limiting the number of connections will not affect request currency.
The newly-added objectStorageSemaphore can work on any object storage despite their protocol.

@reusee reusee requested a review from fengttt July 6, 2024 08:20
@mergify mergify bot merged commit bdcc24e into matrixorigin:main Jul 7, 2024
17 of 18 checks passed
reusee added a commit to reusee/matrixone that referenced this pull request Jul 7, 2024
reusee added a commit to reusee/matrixone that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement size/M Denotes a PR that changes [100,499] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants