Feat: add callback to check how many workers are available #34
Conversation
|
Warning Rate Limit Exceeded@msmakouz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 23 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThis update introduces new functionalities to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
I do not know how to fix the PSALM array return error. Help would be appreciated. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
src/WorkerPool.php
Outdated
| /** | ||
| * Get the worker count for a pool. | ||
| * | ||
| * @param non-empty-string $plugin | ||
| */ | ||
| public function countWorkers(string $plugin): int | ||
| { | ||
| return count($this->getWorkers($plugin)); | ||
| } |
There was a problem hiding this comment.
The implementation of countWorkers directly relies on the getWorkers method to determine the count. This approach is straightforward and leverages existing functionality for consistency. However, consider the performance implications if getWorkers performs heavy operations or fetches a large amount of data. In such cases, a more direct approach to counting, if available through the underlying RPC mechanism, might be more efficient.
src/WorkerPool.php
Outdated
| /** | ||
| * Get the info about running workers for a pool. | ||
| * | ||
| * @param non-empty-string $plugin | ||
| */ | ||
| public function getWorkers(string $plugin): array | ||
| { | ||
| return $this->rpc->call('informer.Workers', $plugin)['workers']; | ||
| } |
There was a problem hiding this comment.
The getWorkers method fetches worker information using the informer.Workers RPC call and directly returns the workers array from the response. This method assumes that the response structure always contains a workers key. To improve robustness, consider adding error handling to manage cases where the response might not contain the expected structure or when the RPC call fails. This could include checking if the workers key exists and handling potential exceptions from the RPC call.
- return $this->rpc->call('informer.Workers', $plugin)['workers'];
+ $response = $this->rpc->call('informer.Workers', $plugin);
+ if (!isset($response['workers'])) {
+ throw new \RuntimeException("Unexpected response structure from informer.Workers");
+ }
+ return $response['workers'];Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| /** | |
| * Get the info about running workers for a pool. | |
| * | |
| * @param non-empty-string $plugin | |
| */ | |
| public function getWorkers(string $plugin): array | |
| { | |
| return $this->rpc->call('informer.Workers', $plugin)['workers']; | |
| } | |
| /** | |
| * Get the info about running workers for a pool. | |
| * | |
| * @param non-empty-string $plugin | |
| */ | |
| public function getWorkers(string $plugin): array | |
| { | |
| $response = $this->rpc->call('informer.Workers', $plugin); | |
| if (!isset($response['workers'])) { | |
| throw new \RuntimeException("Unexpected response structure from informer.Workers"); | |
| } | |
| return $response['workers']; | |
| } |
tests/Unit/WorkerPoolTest.php
Outdated
| public function testCountWorkers(): void | ||
| { | ||
| $this->rpc->expects($this->once())->method('call')->with('informer.Workers', 'test')->willReturn(['workers' => []]); | ||
|
|
||
| $this->workerPool->countWorkers('test'); |
There was a problem hiding this comment.
The testCountWorkers method correctly mocks the informer.Workers RPC call to return an empty array of workers. This test verifies that the countWorkers method can handle an empty worker list. To enhance the test coverage, consider adding more test cases that simulate different scenarios, such as a non-empty list of workers, to ensure the countWorkers method accurately counts the workers under various conditions.
tests/Unit/WorkerPoolTest.php
Outdated
| public function testGetWorkers(): void | ||
| { | ||
| $this->rpc->expects($this->once())->method('call')->with('informer.Workers', 'test')->willReturn(['workers' => []]); | ||
|
|
||
| $this->workerPool->getWorkers('test'); |
There was a problem hiding this comment.
The testGetWorkers method effectively tests the getWorkers method by mocking the informer.Workers RPC call to return an empty array of workers. Similar to the testCountWorkers method, it would be beneficial to expand the test cases to cover scenarios with non-empty worker lists. Additionally, testing error handling and edge cases, such as unexpected response structures, would further ensure the robustness of the getWorkers method.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.x #34 +/- ##
============================================
+ Coverage 48.37% 52.56% +4.19%
- Complexity 94 99 +5
============================================
Files 9 11 +2
Lines 215 234 +19
============================================
+ Hits 104 123 +19
Misses 111 111 ☔ View full report in Codecov by Sentry. |
|
@FluffyDiscord Hi, I have fixed Psalm and made some improvements to your PR. Can I commit the changes here? |
yes, no need to ask :] |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- src/Informer/Worker.php (1 hunks)
- src/Informer/Workers.php (1 hunks)
- src/WorkerPool.php (2 hunks)
- tests/Unit/WorkerPoolTest.php (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/WorkerPool.php
- tests/Unit/WorkerPoolTest.php
Additional comments: 2
src/Informer/Workers.php (1)
- 7-21: The implementation of the
Workersclass and its adherence to theCountableinterface is well done. It provides a clear and straightforward way to manage and count Worker instances. This addition aligns with the PR objectives of enhancing system capabilities without disrupting existing functionality.src/Informer/Worker.php (1)
- 7-29: The
Workerclass is well-structured and provides a clear representation of a worker's state with appropriately typed properties. The use ofpositive-intin PHPDoc comments for certain properties is a good practice, enhancing type safety and documentation clarity. This class effectively supports the new features introduced in the PR, enabling detailed information about workers to be retrieved.
We can add and remove workers, but we don't know how many of them there are in the first place 🙃
Going by the RPC call roadrunner-server/roadrunner#1728
Summary by CodeRabbit