Code review comment for lp:~stub/launchpad/garbo

Thanks for implementing this cleanup. The disk space will be welcome! Daily runs may be a bit much, since it's so unusual for a POTranslation to become unreferenced. Any idea how long the query that searches for orphans will take?

Don't let the Needs Fixing vote put you off—most of the branch looks very good indeed. It's just that I feel the test needs some more work. I do have a few other notes, most in the form of questions because you probably know better than I do, actually.

It seems a bit of a shame to have a procedure just to wrap a FETCH. I think we're all on 8.4 now… have you tried putting the FETCH in a WITH instead? Also, is there no way to fetch n tuples at once?

The CLOSE ALL doesn't belong in BulkPruner.__init__ IMHO. Can't we have a cleanup method called in a "finally:"? If the client dies without passing through there, we wouldn't reuse the connection anyway and the cursor would quietly slip into oblivion.

As for the actual DELETE, doesn't it work with USING?

The reference to POComment is a gentle reminder that that table isn't actually in use. It's empty as well.

And then there's the test. It's a bit unwieldy and worse, relies on sample data. That's the part that needs fixing. It would be easier AFAICS to have two simple end-do-end tests: "create object plus BulkPruner that cleans it up; run; abort; show that the object is gone" and "create object plus BulkPruner that doesn't clean it up; run; show that the object is still there." For much of the rest you can trust LoopTuner test coverage. Batching behaviour would be best verified by a separate test method.

By the way, I suspect ZopelessDatabaseLayer would suffice for this test. Less setup.

Jeroen