-
-
Notifications
You must be signed in to change notification settings - Fork 146
feat: Custom Balancer #590
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
Conversation
Co-authored-by: Robert Nagy <[email protected]>
… 6.12.0 (#453) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
src/index.ts
Outdated
| // We spawn if possible | ||
| // TODO: scheduler will intercept this. | ||
| if (this.workers.size < this.options.maxThreads) { | ||
| this._addNewWorker(); |
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.
How is the worker histogram initialized in that case?
If a balancer query the runtime and that is initialized to zero, the balancer will wrongly pickup only new workers.
The same goes for worker recreation in the error case.
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.
For each new worker, the histogram is instantiated to zero, but the whole WorkerPool has a global histogram that can be queried.
Nevertheless, to query it, you need to have a reference to the Worker Pool (Piscina instance). Maybe worth it to pass a reference to the global Piscina instance so it can query this data
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.
Maybe. Or you can initialize to the pool one if it's not a cumulative histogram.
On poolifier, each entries init to the min of worker matching entries.
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.
That would be useful, I'll follow that path; thanks!
|
|
||
| let signal: AbortSignalAny | null; | ||
| if (this.closingUp) { | ||
| if (this.closingUp || this.destroying) { |
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.
What is the difference between closingUp and destroying flag? I think the two can be merged.
|
Thanks for the feedback @jerome-benoit, I'll kick-off the second round along the week |
|
@ronag I've updated the docs, feel free to take a look now 👍 |
48b898e to
4093dfa
Compare
If you are sure that no regressions are introduced by that PR, you should merge it and flag load balancer feature as experimental. Then do an alpha or beta or ... release and communicate about it to allow people to play with it and do feedbacks. |
|
Agree, that's the plan. After this is merged, I'll start issuing I'll reflect this in the documentation of this PR |
|
I'm merging now, if there's more feedback please write it here, I'll be iterating over it in upcoming PRs Let's land and release some alpha release of the new version to get some feedback; once the Atomics implementation is also merged, I'll release the version. |
Abstract event handling through AsyncResourcePoolworkerornull)