Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feat: add callback to check how many workers are available #34

Merged
merged 5 commits into from
Feb 23, 2024
Merged

Feat: add callback to check how many workers are available #34

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
35697bf
- feat: add callback to check how many workers are available
Feb 21, 2024
3f8f866
- feat: add callback to get worker info
Feb 21, 2024
680ec4f
Improve code, fix psalm and tests
msmakouz Feb 22, 2024
2cd654f
Remove unused phpdoc
msmakouz Feb 22, 2024
6791dc9
Improve comment
msmakouz Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/WorkerPool.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,26 @@ public function addWorker(string $plugin): void
$this->rpc->call('informer.AddWorker', $plugin);
}

/**
* Get the worker count for a pool.
*
* @param non-empty-string $plugin
*/
public function countWorkers(string $plugin): int
{
return count($this->getWorkers($plugin));
}
Copy link

Choose a reason for hiding this comment

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

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.


/**
* 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'];
}
Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
/**
* 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'];
}


/**
* Remove worker from the pool.
*
Expand Down
16 changes: 15 additions & 1 deletion tests/Unit/WorkerPoolTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,24 @@ public function testAddWorker(): void
$this->workerPool->addWorker('test');
}

public function testCountWorkers(): void
{
$this->rpc->expects($this->once())->method('call')->with('informer.Workers', 'test')->willReturn(['workers' => []]);

$this->workerPool->countWorkers('test');
Copy link

Choose a reason for hiding this comment

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

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.

}

public function testGetWorkers(): void
{
$this->rpc->expects($this->once())->method('call')->with('informer.Workers', 'test')->willReturn(['workers' => []]);

$this->workerPool->getWorkers('test');
Copy link

Choose a reason for hiding this comment

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

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.

}

public function testRemoveWorker(): void
{
$this->rpc->expects($this->once())->method('call')->with('informer.RemoveWorker', 'test');

$this->workerPool->removeWorker('test');
}
}
}
Loading