Merge lp:~jtv/launchpad/db-bug-499404 into lp:launchpad/db-devel

= Bug 499404 =

This implements some new classes that will be needed for Rosetta to run jobs on the generalised build farm. The job in question is generating translation templates based on a source-code branch, and will require a lot more work before it goes operational.

Generalisation of the build farm is ongoing work, so its APIs etc. are not complete yet. What you find here is:

1. An implementation of IBuildFarmJob—a new class representing a job that can be dispatched to the build farm. It's called TranslationTemplatesBuildJob. The only "payload" in this new class is a reference to the branch that the job is to check out. There's also a reference to Job, which holds the generic part of the jobbing information.

2. A BuildFarmJobSet utility—to produce and navigate jobs of the new type. It also creates a BuildQueue record for each job it creates; these records are matched to the records of the new type in that they both refer to the same Job record.

3. A new IBuildFarmJobBehavior implementation for dispatching the new job type to the build farm, with an adapter associating it with TranslationTemplatesBuildJob. More may be needed there in the future.

There's not much I can do to test the main IBuildFarmJobBehavior method right now; we'll need more of the building to be operational before we get to that. But work is in progress and I'm filing separate bugs for this.

To test:
{{{
./bin/test -vv -t translationtemplatesbuild
}}}

No lint.

Jeroen

This is still very much work in progress but as such it looks good!

Mainly fine.

I prefer the non-unique index names to have an __idx suffix (avoids some probably non-existant namespace issues).

The ON DELETE CASCADE is an issue. If someone attempts to delete a Branch that has a running job, I suspect we actually want to generate an error (or will things somehow progress just fine despite the branch disappearing part of the way through the process?). There is quite a bit of logic in branch deletion on when branches can be deleted, and the DB constraints serve as a safety net for that. The ON DELETE CASCADE on the job column seems sane.

The DB patch is already using an assigned patch number, so nothing to change there.

> I prefer the non-unique index names to have an __idx suffix (avoids some
> probably non-existant namespace issues).

Done.

> The ON DELETE CASCADE is an issue. If someone attempts to delete a Branch that
> has a running job, I suspect we actually want to generate an error (or will
> things somehow progress just fine despite the branch disappearing part of the
> way through the process?). There is quite a bit of logic in branch deletion on
> when branches can be deleted, and the DB constraints serve as a safety net for
> that. The ON DELETE CASCADE on the job column seems sane.

I didn't feel like weighing down that cleanup logic even further—only to simulate a regular cascade AFAICS. But I'll go the safe route and remove the cascading for now.

The database patch looks good, but it leaves me wondering, where do the results of the job go?

On the code side, the interface that you have there looks an awful lot like IBranchJob -- should it subclass from it? A lot of the branch jobs use IFooJobSource, rather than IFooJobSet. Perhaps it's worth considering using similar names.

jml

> The database patch looks good, but it leaves me wondering, where do the
> results of the job go?

That goes through completely separate channels. The slaves that run these jobs are untrusted, so they're not allowed anywhere near the database. A master-side script will pick up the files they produce (as already happens for the existing Soyuz build jobs) and "upload" them to the regular Translations import queue.

> On the code side, the interface that you have there looks an awful lot like
> IBranchJob -- should it subclass from it? A lot of the branch jobs use
> IFooJobSource, rather than IFooJobSet. Perhaps it's worth considering using
> similar names.

An interesting idea, but this class doesn't have a "metadata" attribute yet. It may still sprout one; at that point it'd be the sensible thing to do. I've added a note.

Fine if we drop ON DELETE CASCADE from the branch foreign key definition.

I don't think this database patch is necessary. The is a branch job, it should use the branch job table.

> Fine if we drop ON DELETE CASCADE from the branch foreign key definition.

And change the SERIAL UNIQUE to SERIAL PRIMARY KEY.

(which we would have picked up when staging failed to rebuild since we can't replicate tables without primary keys).

Using BranchJob may indeed be a better alternative - it already has everything you need. If you want a separate table I think we need some sort of rationalization.

Thinking more about it, I think that Tim is right -- we should use BranchJob for this. There are other branch job implementations with empty metadata dicts -- it's an entirely cromulent approach, and we shouldn't add new tables without a good reason.