feat: Custom Balancer #590
Conversation
Co-authored-by: Robert Nagy <ronagy@icloud.com>
… 6.12.0 (#453) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
| @@ -87,12 +89,25 @@ export class TaskInfo extends AsyncResource implements Task { | |||
|
|
|||
| this.filename = filename; | |||
| this.name = name; | |||
| // TODO: This should not be global | |||
There was a problem hiding this comment.
And it should a UUID to avoid overflow.
There was a problem hiding this comment.
Interesting, do you see a need for universal uniqueness?
My first idea was to provide a simple counter that limits to the SMI optimization and re-starts; maybe in combination with the Worker ID; and if need for enabling more complex use-cases ask the caller to provide an ID generator
There was a problem hiding this comment.
High rate tasks submission workload could overflow it if no restart is done since months. Isn't the task execution counting already done via histogram?
Can it also overflow?
There was a problem hiding this comment.
True, but do we would need to track down every task individually for more than 30 days? I imagine that maybe you'll like it for debugging purposes, but not sure how much after that.
For that, caller should implement a custom solution that fits its use case; I'd like to provide a good defaults with less overhead and allow extensibility.
Isn't the task execution counting already done via histogram?
Can it also overflow?
Noup. I imagine the task id just for debugging purposes, beyond that I don't see much of a use for it.
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.
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.
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.
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.
That would be useful, I'll follow that path; thanks!
| @@ -424,9 +498,9 @@ class ThreadPool { | |||
| filename = maybeFileURLToPath(filename); | |||
|
|
|||
| let signal: AbortSignalAny | null; | |||
| if (this.closingUp) { | |||
| if (this.closingUp || this.destroying) { | |||
There was a problem hiding this comment.
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 👍 |
metcoder95
force-pushed
the
feat/custom_balancer
branch
from
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)