Code review comment for lp:~edwin-grubbs/launchpad/bug-615654-jobqueue-cron-script

Revision history for this message
Gavin Panella (allenap) wrote :

Hi Edwin,

This branch looks good.

I know you're adding ProcessJobSourceGroups in here as well as the
membership stuff, but it still seems like a lot of work to do
something asynchronously. Moving something out of the app server takes
a lot of plumbing work, and it's not trivial plumbing work either.

I'd really like a way to create, in the app server, something like a
persistent closure that can be run later, there or somewhere
else. When run it would have the same user and database access as in
the app server thread that spawned it. Creating a new job now seems to
involve creating tables, database users, job sources, and so on, and
it's getting in the way of the actual work that needs doing.

I'm rambling on, and it's not a fault in this branch, but after doing
this recently, do you have any thoughts on that?

Gavin.

[1]

+ for job_source in selected_job_sources:
+ child = subprocess.Popen(child_args + [job_source])
+ children.append(child)
+ for child in children:
+ child.wait()

Is selected_job_sources ever likely to be large? If not now, but could
be one day, consider filing a bug to limit the number of concurrency
processes.

[2]

+ for child in children:
+ child.wait()

Is there any value in checking and doing something with the exit code
from the child?

[3]

+ packages=('_pythonpath', 'twisted', 'ampoule'), env=env)

I just looked again at _pythonpath.py and now I can't see.

review: Approve

« Back to merge proposal