feat: make ConcurrentLimitLayer accept Semaphore #6618

PsiACE · 2025-10-02T05:54:38Z

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.

Signed-off-by: Chojan Shang <chojan.shang@vesoft.com>

bonsairobo

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

Signed-off-by: Chojan Shang <chojan.shang@vesoft.com>

tisonkun

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 · 2025-10-03T16:45:21Z

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

tisonkun · 2025-10-03T16:46:14Z

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

bonsairobo · 2025-10-03T19:48:26Z

@tisonkun I don't

tisonkun

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 · 2025-10-08T08:05:23Z

@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 · 2025-10-08T08:11:00Z

@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 · 2025-10-08T17:03:21Z

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 · 2025-10-17T08:03:14Z

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

Signed-off-by: Chojan Shang <psiace@apache.org>

core/src/layers/concurrent_limit.rs

Signed-off-by: Chojan Shang <chojan.shang@vesoft.com>

Xuanwo · 2025-11-27T06:04:33Z

bindings/java/src/layer.rs

 ) -> jlong {
    let op = unsafe { &*op };
-    let concurrent_limit = ConcurrentLimitLayer::new(permits as usize);
+    let concurrent_limit = ConcurrentLimitLayer::with_permits(permits as usize);


I don't think we need to this change this API.

Xuanwo · 2025-11-27T06:06:49Z

core/src/layers/concurrent_limit.rs

+/// ConcurrencySemaphore abstracts a semaphore-like concurrency primitive
+/// that yields an owned permit released on drop. It mirrors RetryLayer's
+/// interceptor pattern by serving as a generic extension point for the layer.
+pub trait ConcurrencySemaphore: Send + Sync + Any + Clone + 'static {


How about naming it after the layer like ConcurrentLimitSemaphore?

Xuanwo · 2025-11-27T06:07:44Z

core/src/layers/concurrent_limit.rs

-    pub fn new(permits: usize) -> Self {
+    /// The provided semaphore will be shared by every operator wrapped by this
+    /// layer, giving callers full control over its configuration.
+    pub fn new(operation_semaphore: Arc<Semaphore>) -> Self {


Please don't change public API unless we have to.

Xuanwo · 2025-11-27T06:08:19Z

core/src/layers/concurrent_limit.rs

+    /// Create a new `ConcurrentLimitLayer` with the specified number of
+    /// permits.
+    pub fn with_permits(permits: usize) -> Self {
+        Self::new(Arc::new(Semaphore::new(permits)))
+    }


We can remove this API.

Xuanwo · 2025-11-27T06:08:44Z

core/src/layers/concurrent_limit.rs

+    /// Provide a custom HTTP concurrency semaphore instance.
+    pub fn with_http_concurrency(mut self, semaphore: Arc<S>) -> Self {
+        self.http_semaphore = Some(semaphore);


It's better to always follow the same API naming pattern.

feat: share semaphore with operation_semaphore

cf6cb69

Signed-off-by: Chojan Shang <chojan.shang@vesoft.com>

PsiACE requested a review from Xuanwo as a code owner October 2, 2025 05:54

dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Oct 2, 2025

PsiACE changed the title ~~壮举：与operation_semaphore共享semaphore~~ feat: share semaphore with operation_semaphore Oct 2, 2025

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

Merge branch 'main' into accept-semaphore

6898b93

bonsairobo suggested changes Oct 2, 2025

View reviewed changes

feat: add semaphore to new, more fn

06ce670

Signed-off-by: Chojan Shang <chojan.shang@vesoft.com>

PsiACE requested review from Zheaoli, messense and tisonkun as code owners October 3, 2025 16:32

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 requested a review from bonsairobo October 3, 2025 16:34

PsiACE changed the title ~~feat: share semaphore with operation_semaphore~~ feat: make ConcurrentLimitLayer accept Semaphore Oct 3, 2025

tisonkun reviewed Oct 3, 2025

View reviewed changes

bonsairobo approved these changes Oct 3, 2025

View reviewed changes

tisonkun requested changes Oct 8, 2025

View reviewed changes

refactor: add a trait like retrylayer

dbbffe0

Signed-off-by: Chojan Shang <psiace@apache.org>

Xuanwo reviewed Oct 22, 2025

View reviewed changes

core/src/layers/concurrent_limit.rs Outdated Show resolved Hide resolved

core/src/layers/concurrent_limit.rs Outdated Show resolved Hide resolved

PsiACE added 3 commits November 20, 2025 00:47

Merge branch 'apache:main' into accept-semaphore

e01d093

refactor(concurrent_limit): clean up semaphore trait and impls

b3d1125

Signed-off-by: Chojan Shang <chojan.shang@vesoft.com>

fix(bindings/nodejs): switch concurrent limit layer to with_permits

b8ed310

Signed-off-by: Chojan Shang <chojan.shang@vesoft.com>

PsiACE requested a review from suyanhanx as a code owner November 19, 2025 17:27

PsiACE requested a review from Xuanwo November 19, 2025 17:33

Merge branch 'main' into accept-semaphore

3c3a0d8

Xuanwo reviewed Nov 27, 2025

View reviewed changes

feat: make ConcurrentLimitLayer accept Semaphore #6618

Are you sure you want to change the base?

feat: make ConcurrentLimitLayer accept Semaphore #6618

Conversation

PsiACE commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

bonsairobo left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Oct 3, 2025

Uh oh!

tisonkun commented Oct 3, 2025

Uh oh!

bonsairobo commented Oct 3, 2025

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

bonsairobo commented Oct 8, 2025

Uh oh!

tisonkun commented Oct 8, 2025

Uh oh!

bonsairobo commented Oct 8, 2025

Uh oh!

Xuanwo commented Oct 17, 2025

Uh oh!

Uh oh!

Uh oh!

Xuanwo Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Xuanwo Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Xuanwo Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Xuanwo Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Xuanwo Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PsiACE commented Oct 2, 2025 •

edited

Loading