feat: pool events for workers #624
Conversation
Co-authored-by: Robert Nagy <[email protected]>
src/worker_pool/index.ts
Outdated
| @@ -31,6 +33,8 @@ export class WorkerInfo extends AsynchronouslyCreatedResource { | |||
| lastSeenResponseCount : number = 0; | |||
| onMessage : ResponseCallback; | |||
| histogram: RecordableHistogram | null; | |||
| terminating = false; | |||
| destroyed = false; | |||
There was a problem hiding this comment.
What are the use cases needing to keep track of the worker lifecycle at termination or pool destroying?
The use cases regarding internal worker state signaling:
- Is a worker ready?
- Is a worker errored?
The use cases regarding pool state signaling:
- Is the pool ready?
- ...
I do not see a use case where workers lifecycle need to be signaled to the pool user: a worker pool is meant to guarantee that the submitted task will be executed on a worker with a minimum overhead per design (worker pre-instantiation and preparation, workers lifecycle housekeeping, ... like in an object cache factory design pattern) .
There was a problem hiding this comment.
The use-cases I've in mind was mostly monitoring or possibly to update the load balancer metrics upon request, my fear here is to cause big overhead due to the constant creations of EventEmitter.
Although the API might be better to communicate this, I found enough by stating the current state of the worker, found it less intrusive and easier to keep track. As using getters for it, the state will be updated almost immediately, reflecting the state of the worker at a given point of time.
Maybe I need to add some more to it, but currently I could just think on terminating and destroyed; which can be potentially helpful as there will be race conditions where the balancer might want to distribute the workload to a given worker that its crashing at the same time (reducing the surface but maybe not mitigating it).
There was a problem hiding this comment.
Understood now. Make sense. The implementation is debatable but is inline with the existing events emission one.
LB would need to track worker lifecycle and task(s) lifecycle on a worker.
| @@ -2,6 +2,7 @@ import assert from 'node:assert'; | |||
|
|
|||
| export abstract class AsynchronouslyCreatedResource { | |||
| onreadyListeners : (() => void)[] | null = []; | |||
| ondestroyListeners : (() => void)[] | null = []; | |||
There was a problem hiding this comment.
Why not make AsynchronouslyCreatedResource extends EventEmitter?
There was a problem hiding this comment.
So we emit events with the updated state of the worker?
There was a problem hiding this comment.
The existing code seems to re-eimplement from scratch EventEmitter semantic. I do not know the rationale behind it.
There was a problem hiding this comment.
See what you mean; me neither, I imagine the idea was to implement a lightweight version of the EventEmitter; let me see if there's some potential performance overhead that was tried to be avoided, otherwise can switch to it.
There was a problem hiding this comment.
After replacing, and using an EventEmitter instead, the performance dropped at least 30%. Then it confirms it was mostly due to small use case and to reduce overhead
|
This issue has been marked as stale because it has been opened 45 days without activity. Remove stale label or comment or this will be closed in 10 days. |
No description provided.