-
Notifications
You must be signed in to change notification settings - Fork 105
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
feat: Custom Balancer #590
base: current
Are you sure you want to change the base?
Conversation
Co-authored-by: Robert Nagy <[email protected]>
… 6.12.0 (#453) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Robert Nagy <[email protected]>
Co-authored-by: Robert Nagy <[email protected]>
Co-authored-by: Robert Nagy <[email protected]>
|
||
export type PiscinaTaskBalancer = ( | ||
task: PiscinaTask, | ||
workers: PiscinaWorker[] |
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.
I'm doubtful about wether or not pass the workers on every call or make the balancer rely directly on the events.
By passing them on every call, balancer will always have the up-to-date list of workers, but might cause them to need to constantly adapt and lack possibly the state.
On the event based approach they get definitely more control; maybe keep both?
@ronag @jerome-benoit this should be ready for a review for landing; so far the balancer has been decoupled from an scheduler. I'll update docs and more later this week |
Great! I do not think I will be able to do a review before next week ... |
No rush! I'll proceed with the documentation this week |
this._addNewWorker(); | ||
} | ||
|
||
if (task.abortSignal) { |
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.
if (task.abortSignal) { | |
if (task.abortSignal?.aborted) { |
?
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.
We do that check already in upper layers, if it reaches here, it means the task is not aborted yet.
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.
RSLGTM. A bit too much for me to get a good overview. Would be great with some docs and samples. I think sorting out the public API is higher prio than the implementation details.
Co-authored-by: Robert Nagy <[email protected]>
SGTM, I'll start preparing either Tomorrow or later in the weekend |
Abstract event handling through AsyncResourcePoolworker
ornull
)