Code review comment for ~ilasc/turnip:stats-collection-point

Revision history for this message
Colin Watson (cjwatson) wrote :

You were somewhat close with your option 3, but you had the extra file descriptor backwards. I'm expecting something more along the lines of this:

  https://paste.ubuntu.com/p/xB8TshVHj6/

Things to note:

 * the supervising process protocol arrangement remains essentially the same, with only the addition of handling an extra pipe for resource usage data;
 * we interpose an extra process between turnip-pack-backend and git, whose job is to collect resource usage information from the child and write it to a dedicated pipe
 * because we have an extra *process* (not a thread or something in-process in Twisted), we can reliably use getrusage(RUSAGE_CHILDREN) to collect *only* resource usage information about the git child process (and any descendant processes);
 * the data format for sending resource usage information back to turnip-pack-backend is private and can be whatever you like as long as it can be parsed reliably; I just used JSON since that was trivial to implement;
 * I had some trouble with the functional tests due to lots of UncleanReactorError exceptions; this looks like some subtle pre-existing race condition rather than something I've introduced, since it had a bad habit of going away when I tried to investigate using strace, so I just whacked it with AsynchronousDeferredRunTestForBrokenTwisted to spin the reactor another couple of times at the end of the test; this isn't ideal, but I wouldn't particularly mind it ending up on master;
 * I haven't implemented the statsd interaction, nor worked out how to label the metrics with information about the repository in question; that's important to do, but I stopped once I was satisfied that I had the process plumbing essentially right, and you should be able to take it from here.

review: Needs Resubmitting

« Back to merge proposal