Skip to content

Conversation

@PsiACE
Copy link
Member

@PsiACE PsiACE commented Oct 2, 2025

Which issue does this PR close?

Closes #6591 . also cc @bonsairobo for review

Rationale for this change

Expose the internal semaphore used by ConcurrentLimitLayer so external components can coordinate concurrency
with OpenDAL without reinventing their own limiters.

What changes are included in this PR?

  • Changed ConcurrentLimitLayer::new to take an Arc<Semaphore> and added the with_permits helper to preserve the prior ergonomic constructor.
  • Added with_http_semaphore plus a documented with_http_concurrent_limit convenience to allow sharing HTTP semaphores as well.
  • Updated Python, Ruby, and Java bindings (and docs/tests) to use the new API surface.

Are there any user-facing changes?

Yes. Applications can now reuse ConcurrentLimitLayer’s semaphore directly to align their own concurrency
controls with OpenDAL.

@PsiACE PsiACE requested a review from Xuanwo as a code owner October 2, 2025 05:54
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Oct 2, 2025
@PsiACE PsiACE changed the title 壮举:与operation_semaphore共享semaphore feat: share semaphore with operation_semaphore Oct 2, 2025
@dosubot dosubot bot added the releases-note/feat The PR implements a new feature or has a title that begins with "feat" label Oct 2, 2025
Copy link
Contributor

@bonsairobo bonsairobo left a comment

Choose a reason for hiding this comment

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

We need to be able to pass the Semaphore to ConcurrentLimitLayer::new so users can have full control over its initialization.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Oct 3, 2025
@PsiACE PsiACE requested a review from bonsairobo October 3, 2025 16:34
@PsiACE PsiACE changed the title feat: share semaphore with operation_semaphore feat: make ConcurrentLimitLayer accept Semaphore Oct 3, 2025
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Since now the Semaphore is in the public API, I wonder if we can use mea's Semaphore that fixes several issues where tokio's Semaphore has.

It's tested in ScopeDB's production environment and should be solid for use.

See https://docs.rs/mea/latest/mea/semaphore/struct.Semaphore.html and https://github.com/fast/mea?tab=readme-ov-file#history

@tisonkun
Copy link
Member

tisonkun commented Oct 3, 2025

Ummm I can see the original issue is about using user's tokio Semaphore now. Let me think a bit ...

@tisonkun
Copy link
Member

tisonkun commented Oct 3, 2025

@bonsairobo do you have a public example how a Semaphore would be shared in this case?

@bonsairobo
Copy link
Contributor

@tisonkun I don't

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thanks for your feedback. Then I'd block this PR until we find a proper use case. Otherwise it does no benefits to tight our interfaces with a certain sync utils impls.

@bonsairobo
Copy link
Contributor

@tisonkun You asked if I have a public example. I don't, but my company has a private repository that would make use of this feature.

it does no benefits to tight our interfaces with a certain sync utils impls.

That's not true. While I agree that there is tight coupling, that doesn't imply that no one uses this type of semaphore.

@tisonkun
Copy link
Member

tisonkun commented Oct 8, 2025

@bonsairobo Thanks for your clarification. Could you elaborate a bit how the semaphore share between different resources and how it benefits your use case?

@bonsairobo
Copy link
Contributor

Could you elaborate a bit how the semaphore share between different resources and how it benefits your use case?

@tisonkun OpenDAL is not the only code in our system that opens files. Processes have an open file limit, which is especially low on MacOS (256). We use a global semaphore to limit open file handles. It only works if all code shares that semaphore, so we need to create it ourselves and feed it into the concurrent limit layer.

@Xuanwo
Copy link
Member

Xuanwo commented Oct 17, 2025

I'm thinking of provide a trait like we do for other layers like RetryLayer. We can allow users to pass any Semaphore they want in the given layer. Do you want to work on this way? @PsiACE

@PsiACE PsiACE requested a review from suyanhanx as a code owner November 19, 2025 17:27
@PsiACE PsiACE requested a review from Xuanwo November 19, 2025 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/feat The PR implements a new feature or has a title that begins with "feat" size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

new feature: ConcurrentLimitLayer should accept a Semaphore

4 participants