gen_magic icon indicating copy to clipboard operation
gen_magic copied to clipboard

Adding NimblePool as a supervisor child

Open maartenJacobs opened this issue 2 years ago • 5 comments

The README states that the following should work:

children = [
  {GenMagic.Pool.NimblePool, pool_name: MyApp.GenMagicPool, pool_size: 2}
]

opts = [strategy: :one_for_one, name: MyApp.Supervisor]
Supervisor.start_link(children, opts)

But Supervisor runs child_spec/1 on its children, giving me this error:

** (Mix) Could not start application gen_magic_test: exited in: GenMagicTest.Application.start(:normal, [])
    ** (EXIT) an exception was raised:
        ** (ArgumentError) The module GenMagic.Pool.NimblePool was given as a child to a supervisor
but it does not implement child_spec/1.

I'm happy to provide a PR. I think there are at least possible 2 solutions:

  • The Pool behaviour requires child_spec/1.
  • The README is updated to include the child spec:
%{
  id: GenMagic.Pool.NimblePool,
  start: {GenMagic.Pool.NimblePool, :start_link, [[pool_name: MyApp.GenMagicPool, pool_size: 2]]}
}

maartenJacobs avatar Jul 13 '23 14:07 maartenJacobs

Hi @maartenJacobs the NimblePool implementation has no child spec, the Poolboy implementation has it, so the README is wrong. I’ll look into it however would suggest using Poolboy for now.

evadne avatar Jul 13 '23 14:07 evadne

@maartenJacobs Please test feature/pool-test latest @ f81bb829395e0a0a36281f8c7e48de13186f5ced and advise, reference the relevant test files for example use cases.

evadne avatar Jul 13 '23 14:07 evadne

@evadne I think you should write @maartenJacobs a cheque for £1 ;-)

Yes comparing you to Knuth - the reference https://en.wikipedia.org/wiki/Knuth_reward_check

devstopfix avatar Jul 13 '23 18:07 devstopfix

@maartenJacobs Please test feature/pool-test latest @ f81bb82 and advise, reference the relevant test files for example use cases.

@evadne @ [f81bb82] works as expected 🙌 The pool size default works as well.

maartenJacobs avatar Jul 14 '23 09:07 maartenJacobs

The pool size default works as well.

@maartenJacobs will we be using a pool size of 2 ;-)

devstopfix avatar Jul 14 '23 09:07 devstopfix