Skip to content

About kill-zk-kafka branch (14367f22)

A comment regarding the 14367f22 change:

It's well justified why someone might not want to wait for Kafka to terminate and I understand it.

Regarding the commit message: request.addfinalizer doesn't add a __del__ method but a Pytest finalizer, which gets executed on teardown. So the commit message line about not waiting for GC is incorrect.

Regarding the code, instead of simply adding zk_proc.kill() (and then zk_proc.wait(), which right now happens in the finalizer), I'd suggest:

A) Changing the _teardown function to also accept a signal number, e.g. signal=SIGTERM by default, and allowing the user to choose the signal when creating their fixture.

This however has a problem with sending signal=SIGKILL to the root process when we know nothing about its potential children. We don't want to leave orphans so we really should send SIGKILL to the entire process group (with os.killpg). If passing signal=SIGTERM however, we don't want to send it to every process in the group.

I don't think Kafka/ZK normally have child processes but I don't like to assume otherwise.

B) Extracting the process tree reaping part of _teardown into a separate function and exposing both as a public interface, so the users can pass them to make_*_fixture.

In both cases, I believe, the final proc.wait() should change into a construct to wait for a every potential child process (with a loop with waitid in it?). This however can be a separate change.