Code review comment for lp:~abentley/launchpad/restore-queue-test

Revision history for this message
Aaron Bentley (abentley) wrote :

> Thanks for the update Aaron.
>
> Only comments are would it make sense for the db user to be an attribute of
> the job class for the _init_task() call so that there's some context on what
> that string is.

That's not a Job class, it's a native Celery Task class. Job classes do indeed have a dbuser and use that for init_task.

> Would the noop task be more a test helper and be specific to that vs in the
> list of normal jobs or is it logical that someone might want to use it for
> actual regular processing at some point?

Yes. All of the tasks in celery_helpers are test helpers, and are only available during test runs.

> Finally, the TransactionFreeOperation is a bit confusing as I'd not expect the
> transaction.commit at the end of something that's not a transaction? Looking
> at the object, it appears to just be something that says there aren't any
> current active transactions, so is this just guarding against multiple
> transactions stomping on each other? Since this is copied from the old code
> and not pertaining the queues, marking approved but appreciate any education
> lesson on this front.

It's ensuring that the operation does not leave any open transactions, because that would prevent other operations from acquiring locks and eventually force them to time out. You're right that the name is a bit confusing.

« Back to merge proposal