inaka / worker_pool

Erlang worker pool
https://hex.pm/packages/worker_pool
Apache License 2.0
276 stars 80 forks source link

one_for_all strategy with the worker pool supervisor #64

Closed waisbrot closed 8 years ago

waisbrot commented 8 years ago

This is a bit long, because I'm not sure what solution I'd like. I have the following situation:

There is a single producer (reads an AWS Kinesis stream) feeding a set of consumers (a worker_pool that writes the data to the DB and to S3).

The producer produces data, and also checkpoint requests. The consumers consume the data, and when they've fully consumed everything up to the checkpoint request, they notify the producer. The producer needs to see responses from all workers before it can checkpoint.

My problem is that if a single worker dies I want it to restart the producer, doing one_for_all across the pool of workers and the producer.

When I create the worker_pool, I request a restart intensity of 0 so that any worker failure restarts the pool supervisor. However, this doesn't help because there's a supervisor above that which I can't configure: https://github.com/inaka/worker_pool/blob/master/src/wpool_pool.erl#L345

Here are a few ideas for things that would fix this for me

  1. Make the wpool_pool supervisor configurable, as well as the wpool_process_sup supervisor. This would be easy, but seems confusing and ugly to have two config options for supervisors.
  2. Have the wpool_pool supervisor restart at 0 intensity. This could be annoying if wpool_time_checker or wpool_queue_manager were crashing.
  3. Export a function that makes the childspec for wpool_pool, so I can attach it to my supervisor directly. Same problem as 2, though the crash output would look a little different.

Any other thoughts? I'm happy to write the code if you've got a solution that you like. Or am I just misusing worker_pool or OTP?

elbrujohalcon commented 8 years ago

Hi @waisbrot,

That supervisor strategy was never configurable because we didn't want people replacing one_for_all by any other thing. We need one_for_all for some of our strategies to work properly. But you're right, the other 2 elements in that tuple can be open to user configuration. I would go with your option 1, but I would not let users pass in a full strategy tuple. Instead I would allow them to pass in the restart intensity and period values only. Would that be ok with you?