-
Notifications
You must be signed in to change notification settings - Fork 95
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.
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
Use worker pool for smartctl #204
base: master
Are you sure you want to change the base?
Conversation
…theus-community#197). Signed-off-by: Póka Balázs <[email protected]>
@NiceGuyIT could you comment on whether I need to do something else before this PR is likely to be accepted? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
return | ||
} | ||
|
||
pool := createPool() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be implementing a sync.WaitGroup
. Or maybe better, use errgroup.Group
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Will try to refactor it later, after the fate of #205 is decided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Will try to refactor it later, after the fate of #205 is decided.
Merged. Can you fix the conflicts for merge?
Use worker pool to make running of smartctl parallel (for issue #197).