Code review comment for lp:~vila/udd/735477-kill-harder

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 3/17/2011 5:39 PM, Vincent Ladeuil wrote:
>
>> I think 2 seconds is a bit short for a grace period. If bzr is doing
>> anything significant.
>
> Keep in mind that this grace period is between 2 kills not between the job start and the kill.
>
> There is actually two situations where we kill imports:
> - they have exceeded their time quota,
> - the mass_import script is killed and should stop as soon as possible.
>
>> Something like 1 minute would be generous,
>
> losas will hate us if we make them wait 1 minute just because.
>

I think something like 10s is reasonable to give it a chance to shutdown
cleanly, rather than killing it and leaving locks open, etc. Remember,
by this point the system is probably under load, and may take a bit to
context switch. I think 2s is a bit short, but we can live with it.

>> and
>> still avoid having a child sitting around for days before manually
>> killing it.
>
> For the nexuis-data case, the issue is that SIGTERM is not honored so again, there is no point in waiting, only SIGKILL will do.
>
>> Why not use 'mkfifo' and then actually do inter-process communication,
>> instead of playing tricks with file content. mkfifo + open() will block
>> until the file actually is opened by the other side.
>
> Reliability. I don't want to test mkfifo nor dive into whatever problems I can encounter with inter-process communications nor python/Ubuntu specifics. These tricks are used only in the tests and are easy enough to grasp for test writers.
>

Honestly, I think mkfifo is going to be significantly more reliable. It
is what it was made for, versus hacking around getting half the text
written to a file, or any other such thing for a regular file.

>>
>> If you are worried that they'll never open the file and you'll block.
>> You can O_NONBLOCK and poll like you are doing now. Or use something
>> like signal.alarm() so that the test suite dies if the file never opens.
>> (You could even install a SIGALRM handler and just have EINTR triggered
>> on the open() call.)
>
> No, no, no, signals and threads can't be mixed (and even if they could be used in this particular case, I'd prefer to avoid them as long as possible).
>

Except you're already using signals...

John
=:->

>>
>> Otherwise something like this seems fine. I certainly support falling
>> back to SIGKILL if SIGTERM/SIGINT aren't doing their job.
>>
>> review: needsfixing
>> merge: approve
>
> Thanks.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2CTQ8ACgkQJdeBCYSNAAMmegCdGPeee4xg+4hIrX9bIaLD2EO3
bBsAniA64QsHqA2M709fcx+Iv6GW9VQL
=LWVa
-----END PGP SIGNATURE-----