refactor: add concurrency as configurable option for semaphore#54
refactor: add concurrency as configurable option for semaphore#54
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## refactor #54 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 5 5
Lines 591 591
==========================================
Hits 591 591 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors semaphore initialization by introducing a new configurable concurrency parameter.
- Adds a "concurrency" parameter (defaulting to 32) to the constructor.
- Updates the semaphore initialization to use the configurable concurrency value.
| session: aiohttp.ClientSession | None = None, | ||
| rpc_base_url: str | None = KUBO_DEFAULT_LOCAL_RPC_BASE_URL, | ||
| gateway_base_url: str | None = KUBO_DEFAULT_LOCAL_GATEWAY_BASE_URL, | ||
| concurrency: int = 32, |
There was a problem hiding this comment.
Consider updating the constructor's docstring to include a description of the new 'concurrency' parameter, detailing its purpose and expected value range.
| self._owns_session = True # we’ll create sessions lazily | ||
|
|
||
| self._sem: asyncio.Semaphore = asyncio.Semaphore(64) | ||
| self._sem: asyncio.Semaphore = asyncio.Semaphore(concurrency) |
There was a problem hiding this comment.
Consider adding input validation for the 'concurrency' parameter to ensure that it is a positive integer, as non-positive values could lead to unexpected behavior.
There was a problem hiding this comment.
i remember testing this and not having much effect. But better to have the option for the future anyways
No description provided.