Do not start no_name workers under KafkaEx.Supervisor #350
Conversation
|
Ebert has finished reviewing this Pull Request and has found:
You can see more details about this review at https://ebertapp.io/github/kafkaex/kafka_ex/pulls/350. |
| def start_worker(name, worker_init \\ []) do | ||
| case build_worker_options(worker_init) do | ||
| {:ok, worker_init} -> | ||
| apply(KafkaEx.Config.server_impl, :start_link, [worker_init, name]) |
There was a problem hiding this comment.
I'm not sure about this. Even without even discussing whether we want to break up the supervision tree like this, I think it is going to cause a lot of confusion when people report these sorts of supervision tree errors, and we have to determine which worker creation function is being used, and deal with supervision trees behaving differently based on that.
I'm certainly open to changing the supervision tree, but we should probably chain it over entirely, and we should also do something to verify that this fixes the issue that you saw.
There was a problem hiding this comment.
I agree, this is a very big change. I don't really love the way our supervision tree works, but this would break a lot of people's code.
|
Closing due to lack of follow-up/proof and how large of an API behavior change this would be. The new The supervision strategy will probably be revisited for the v1.0 design as well. |
3 participants
I'm sorry that I can't prove, but my colleges told me that they had connection leakage with kafka_ex in case of software or network errors. So I decided that it would be better to link connection process directly to consumer or manager. In this case there is no chance for connection process to stay alive after consumer or manager process crash. Also I can’t find any disadvantages of starting worker without supervisor. And it doesn’t look good to have connection and consumers started under different applications.
Process.unlink(state.worker_name)seems useless, because commit has already done andGenServer.stopwill stop worker with normal reason.Manual worker creation described in README.md also become simpler.