Conversation
Signed-off-by: Chojan Shang <psiace@apache.org>
There was a problem hiding this comment.
Pull request overview
This PR adds support for custom/shared semaphores to ConcurrentLimitLayer without breaking the existing API. It introduces a new ConcurrentLimitSemaphore trait abstraction that allows users to provide their own semaphore implementations while maintaining backward compatibility with the existing new(permits) constructor.
Key changes:
- Introduced
ConcurrentLimitSemaphoretrait to abstract semaphore implementations - Added
with_semaphore()andwith_http_semaphore()methods for custom semaphore injection - Refactored internal types to be generic over the semaphore implementation
- Added tests demonstrating shared and custom semaphore usage
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
core/layers/concurrent-limit/src/lib.rs |
Core implementation: added ConcurrentLimitSemaphore trait, made layer/accessor/wrapper types generic over semaphore type, added new API methods, and included tests for shared and custom semaphores |
core/layers/concurrent-limit/Cargo.toml |
Added tokio as dev-dependency for async test support |
core/Cargo.lock |
Updated lock file to reflect new tokio dev-dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Chojan Shang <psiace@apache.org>
|
The failed CI appears unrelated to this PR. |
Xuanwo
left a comment
There was a problem hiding this comment.
Thank you for working on this!
|
The only thing left is to resolve the conflicts, would you like to take another look? |
Which issue does this PR close?
Closes #6591 .
Port #6618 with fixes.
Rationale for this change
Add support for passing a shared/custom semaphore to ConcurrentLimitLayer without breaking existing APIs.
What changes are included in this PR?
Are there any user-facing changes?
Yes. ConcurrentLimitLayer can now accept a custom/shared semaphore via with_semaphore(...) while keeping the existing new(permits) constructor.
AI Usage Statement
Implemented with OpenAI Codex (GPT-5) via Codex CLI.