Merge lp:~jtv/launchpad/bug-146855 into lp:launchpad

[Jeroen T. Vermeulen]

[Jeroen T. Vermeulen (jtv)]

= Bug 146855 =

Actually this branch resolves more long-standing gripes with the
translations import auto-approver than just that one bug.

But let's start with the original bug. An entry on the translations
import queue can be in one of several states: Needs Review, Approved,
Imported, Failed, Blocked, or Deleted. Failed is the state it ends up
in when its import failed, typically because of a syntax error.

Successfully imported entries get gc'ed off the queue after a few days.
Currently, Failed ones do not. The idea is that its owner uploads an
updated copy of the same file; it reuses the same queue entry; it gets
processed; it ends up successfully Imported; and so finally the entry
drops out of the queue. But in practice, especially for Ubuntu, Failed
entries lie around effectively forever.

So I fixed that. We want to clean up these entries less aggressively
than we do Imported ones, so the owner gets a fair chance to notice and
fix the problem (and sometimes, so that we get to restart a bunch of
entries that failed because of operational problems). This branch makes
the garbage-collection of entries in various states data-driven: 3 days
for Imported or Deleted entries, one month for Failed ones. There is no
need to test each state's "grace period" separately since we'd only be
unit-testing the contents of a dict.

== Other gripes ==

The approver's LaunchpadCronScript still lived in the same module as the
script it was originally spliced out of. Bit of a Zeus-and-Athena
scenario there. I gave it its own module in lp.translations.scripts.

Also, the class in there now inherits from LaunchpadCronScript instead
of having it instantiated from a separate LaunchpadCronScript-derived
class in the main script file. Unfortunately this did move some debug
output into the doctest; I switched from the FakeLogger to the
MockLogger so I could set a higher debug level and avoid this.

You may noticed that where transaction managers are passed around, I
changed their names from ztm (Zopeless Transaction Manager) to txn. We
don't use the ztm any more. Actually txn is just an alias for the
transaction module, so the argument isn't needed at all now. But this
is a relatively elegant way of telling a method whether you want it to
commit transactions. Passing a boolean for "please commit" is just too
ugly.

Oh, and another biggie: entries for obsolete distroseries. Right now
perhaps a fifth of the queue (certainly over 25K entries, out of 150K!)
are for obsolete Ubuntu releases that will never need any further
translations updates--they won't even get security updates. So I made
the script do some cleanup on those entries.

Deleting all of these would cause major database mayhem every time an
Ubuntu release slipped into obsolescence, so this cleanup is limited to
100 entries per run. Experience shows that that's not a noticeable load
for database replication, yet it'll get rid of all of these entries in a
few days. I didn't bother testing this since it's an operational detail
but I did consistently test that none of these cleanups affect rows that
shouldn't be affected.

To test:
{{{
./bin/test -vv -t poimport -t approval
}}}

No lint.

...

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jeroen T. Vermeulen wrote:
> Actually this branch resolves more long-standing gripes with the
> translations import auto-approver than just that one bug.

Was there a preimplementation call?

> But let's start with the original bug. An entry on the translations
> import queue can be in one of several states: Needs Review, Approved,
> Imported, Failed, Blocked, or Deleted. Failed is the state it ends up
> in when its import failed, typically because of a syntax error.
>
> Successfully imported entries get gc'ed off the queue after a few days.
> Currently, Failed ones do not. The idea is that its owner uploads an
> updated copy of the same file; it reuses the same queue entry; it gets
> processed; it ends up successfully Imported; and so finally the entry
> drops out of the queue. But in practice, especially for Ubuntu, Failed
> entries lie around effectively forever.
>
> So I fixed that. We want to clean up these entries less aggressively
> than we do Imported ones, so the owner gets a fair chance to notice and
> fix the problem (and sometimes, so that we get to restart a bunch of
> entries that failed because of operational problems). This branch makes
> the garbage-collection of entries in various states data-driven: 3 days
> for Imported or Deleted entries, one month for Failed ones. There is no
> need to test each state's "grace period" separately since we'd only be
> unit-testing the contents of a dict.
>
>
> == Other gripes ==
>
> The approver's LaunchpadCronScript still lived in the same module as the
> script it was originally spliced out of. Bit of a Zeus-and-Athena
> scenario there. I gave it its own module in lp.translations.scripts.

> Also, the class in there now inherits from LaunchpadCronScript instead
> of having it instantiated from a separate LaunchpadCronScript-derived
> class in the main script file.

Do you think AutoApproveProcess is still a good name, since it's also
responsible for other queue maintenance? What about ProcessImportQueue
or something?

> Unfortunately this did move some debug
> output into the doctest; I switched from the FakeLogger to the
> MockLogger so I could set a higher debug level and avoid this.
>
> You may noticed that where transaction managers are passed around, I
> changed their names from ztm (Zopeless Transaction Manager) to txn. We
> don't use the ztm any more. Actually txn is just an alias for the
> transaction module, so the argument isn't needed at all now. But this
> is a relatively elegant way of telling a method whether you want it to
> commit transactions. Passing a boolean for "please commit" is just too
> ugly.

But functionally equivalent? (Would we ever pass in a different
transaction manager?)

> Oh, and another biggie: entries for obsolete distroseries. Right now
> perhaps a fifth of the queue (certainly over 25K entries, out of 150K!)
> are for obsolete Ubuntu releases that will never need any further
> translations updates--they won't even get security updates. So I made
> the script do some cleanup on those entries.
>
> Deleting all of these would cause major database mayhem every time an
> Ubuntu releas...

[Jeroen T. Vermeulen (jtv)]

> Jeroen T. Vermeulen wrote:
> > Actually this branch resolves more long-standing gripes with the
> > translations import auto-approver than just that one bug.
>
> Was there a preimplementation call?

Not as such. The approach is pretty straightforward and we've been
mulling some of these points for years. I don't particularly see anything
that raises questions of approach or direction. We did go into what I'm
doing in this branch a few times in our meetings.

> > == Other gripes ==
> >
> > The approver's LaunchpadCronScript still lived in the same module as the
> > script it was originally spliced out of. Bit of a Zeus-and-Athena
> > scenario there. I gave it its own module in lp.translations.scripts.
>
>
> > Also, the class in there now inherits from LaunchpadCronScript instead
> > of having it instantiated from a separate LaunchpadCronScript-derived
> > class in the main script file.
>
> Do you think AutoApproveProcess is still a good name, since it's also
> responsible for other queue maintenance? What about ProcessImportQueue
> or something?

It wasn't a good name when I first saw it, to be honest. You're right, and
I'm changing this. See below.

> > Unfortunately this did move some debug
> > output into the doctest; I switched from the FakeLogger to the
> > MockLogger so I could set a higher debug level and avoid this.
> >
> > You may noticed that where transaction managers are passed around, I
> > changed their names from ztm (Zopeless Transaction Manager) to txn. We
> > don't use the ztm any more. Actually txn is just an alias for the
> > transaction module, so the argument isn't needed at all now. But this
> > is a relatively elegant way of telling a method whether you want it to
> > commit transactions. Passing a boolean for "please commit" is just too
> > ugly.
>
> But functionally equivalent? (Would we ever pass in a different
> transaction manager?)

Functionally equivalent to a boolean, yes; if we ever pass a different
transaction manager then it'll be hidden somewhere under these interfaces
anyway.

Operational note: I've checked with Stuart and it seems that there are no
big replication problems to be expected from deleting our entire backlog of
Failed entries. Otherwise we'd have to clean up the queue a bit before
this change could roll out.

> > === modified file 'cronscripts/rosetta-approve-imports.py'
> > --- cronscripts/rosetta-approve-imports.py 2009-07-17 00:26:05 +0000
> > +++ cronscripts/rosetta-approve-imports.py 2009-09-18 07:39:51 +0000
> > @@ -10,26 +10,10 @@
> > import _pythonpath
> >
> > from canonical.config import config
> > -from canonical.database.sqlbase import ISOLATION_LEVEL_READ_COMMITTED
> > -from lp.translations.scripts.po_import import AutoApproveProcess
> > -from lp.services.scripts.base import LaunchpadCronScript
> > -
> > -
> > -class RosettaImportApprover(LaunchpadCronScript):
> > - def main(self):
> > - self.txn.set_isolation_level(ISOLATION_LEVEL_READ_COMMITTED)
> > - process = AutoApproveProcess(self.txn, self.logger)
> > - self.logger.debug('Starting auto-approval of translation imports')
> > - process.run()
> > - self.logger....

[Jeroen T. Vermeulen (jtv)]

=== modified file 'cronscripts/rosetta-approve-imports.py'
--- cronscripts/rosetta-approve-imports.py 2009-09-18 07:39:51 +0000
+++ cronscripts/rosetta-approve-imports.py 2009-09-21 19:14:06 +0000
@@ -9,11 +9,11 @@

 import _pythonpath

-from canonical.config import config
-from lp.translations.scripts.import_approval import AutoApproveProcess
+from lp.translations.scripts.import_queue_gardener import ImportQueueGardener

 if __name__ == '__main__':
- script = AutoApproveProcess(
- 'rosetta-approve-imports', dbuser=config.poimport.dbuser)
+ script = ImportQueueGardener(
+ 'translations-import-queue-gardener',
+ dbuser='translations_import_queue_gardener')
     script.lock_and_run()

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2009-09-02 19:06:19 +0000
+++ database/schema/security.cfg 2009-09-21 18:55:31 +0000
@@ -435,13 +435,29 @@
 public.account = SELECT, INSERT
 public.customlanguagecode = SELECT
 public.translationgroup = SELECT
-public.translationimportqueueentry = SELECT, DELETE
+public.translationimportqueueentry = SELECT
 public.translationmessage = SELECT, INSERT, UPDATE
 public.translationrelicensingagreement = SELECT
 public.translator = SELECT
 public.validpersoncache = SELECT
 public.validpersonorteamcache = SELECT

+[translations_import_queue_gardener]
+# Translations import queue management
+type=user
+groups=script
+public.customlanguagecode = SELECT
+public.distribution = SELECT
+public.distroseries = SELECT
+public.language = SELECT
+public.person = SELECT
+public.pofile = SELECT
+public.potemplate = SELECT
+public.product = SELECT
+public.productseries = SELECT
+public.sourcepackagename = SELECT
+public.translationimportqueueentry = SELECT, DELETE, UPDATE
+
 [poexport]
 # Rosetta export script
 type=user

=== modified file 'lib/lp/translations/doc/poimport.txt'
--- lib/lp/translations/doc/poimport.txt 2009-09-18 13:48:00 +0000
+++ lib/lp/translations/doc/poimport.txt 2009-09-21 18:58:48 +0000
@@ -16,8 +16,8 @@
     >>> from lp.registry.model.sourcepackagename import SourcePackageName
     >>> from lp.translations.model.potemplate import POTemplateSubset
     >>> from lp.translations.scripts.po_import import ImportProcess
- >>> from lp.translations.scripts.import_approval import (
- ... AutoApproveProcess)
+ >>> from lp.translations.scripts.import_queue_gardener import (
+ ... ImportQueueGardener)
     >>> import datetime
     >>> import pytz
     >>> UTC = pytz.timezone('UTC')
@@ -674,7 +674,7 @@
     <BLANKLINE>
     The Launchpad team

-Now the auto-approval script runs. This can happen anytime, since it's
+Now the queue gardener runs. This can happen anytime, since it's
 asynchronous to the po-import script. The script tries to approve any
 entries that have not been approved, but look like th...

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 review approve

Approved conditionally on switching the database user in the test case
(see below)

Jeroen T. Vermeulen wrote:
>> Jeroen T. Vermeulen wrote:
>> Do you think AutoApproveProcess is still a good name, since it's also
>> responsible for other queue maintenance? What about ProcessImportQueue
>> or something?
>
> It wasn't a good name when I first saw it, to be honest. You're right, and
> I'm changing this. See below.

I like it.

>> It seems a bit odd to be wrapping the script in if __name__ = '__main__'
>> blocks. It can't be loaded as a module because of its name, and even if
>> it could, I don't see any value in it.
>
> It's standard practice; I believe some Python syntax checkers will fool
> themselves into thinking the script has an importable name. I know
> that at least one—don't remember which—does its work by importing even
> a main executable Python file as a module.

Fair enough.

>> Does it make sense to specify a timedelta here? If you always specify
>> your durations in days, it's less repetitive convert to a timedelta in
>> _cleanUpObsoleteEntries
>
> I thought it was nice and explicit. Storing number-of-days struck me as
> unnecessarily C-like; timedeltas let me store a nondescript "time
> interval" that you can just add to or subtract from dates. And who
> knows, maybe someday we'll want other kinds of intervals.

There often is a tension between being explicit and being succinct. I'm
fine either way.

>> If you're specifying a timedelta, shouldn't the comment say "Length of
>> time" instead of "Number of days"? Is FAILED really a terminal status?
>> It seems like it's possibly not terminal, and that's why you're
>> delaying 30 days.
>
> Absolutely. I should have updated that part of the comment as well now.

Hmm. I think you missed the "Is FAILED really a terminal status?"
question. Not that fixing a comment is worth delaying the branch.

>> I find the way the deletion_criteria is constructed a bit surprising.
>> What do you think about building up a list of clauses first, and then
>> ORing them? For example:

> I considered that and felt that it was a toss-up. But I'm used my ways
> of doing things, and now that you mention it, your way is clearer.
> Changed.

Cool.

>>> === added file 'lib/lp/translations/scripts/import_approval.py'
>> Since this does more than approving imports, is import_approval a good
>> name? Maybe queue_gardener?
>
> I renamed it import_queue_gardener.py, and the class ImportQueueGardener.

Great.

> While I was at it I also gave the script its own database user, which
> requires a lot fewer database privileges than the original one.

That change also looks good, but I'd like to see the database user being
set to the new value in the unit tests. IME, it's very easy to miss a
required DB permission unless you test with the script user.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkq32UUACgkQ0F+nu1YWqI39DQCeMlPGurWQ4CxwplR0CaKKBVJA
SgAAn3+54mFu6LLI2TXnVixy0LKGG9/O
=6+zS
-----END PGP SIGNATURE-----

review: Approve

=== modified file 'cronscripts/rosetta-approve-imports.py'
--- cronscripts/rosetta-approve-imports.py	2009-08-04 09:24:21 +0000
+++ cronscripts/rosetta-approve-imports.py	2009-09-26 06:25:27 +0000
@@ -9,27 +9,11 @@
 
 import _pythonpath
 
-from canonical.config import config
-from canonical.database.sqlbase import ISOLATION_LEVEL_READ_COMMITTED
-from lp.translations.scripts.po_import import AutoApproveProcess
-from lp.services.scripts.base import LaunchpadCronScript
-
-
-class RosettaImportApprover(LaunchpadCronScript):
-    def main(self):
-        self.txn.set_isolation_level(ISOLATION_LEVEL_READ_COMMITTED)
-        process = AutoApproveProcess(self.txn, self.logger)
-        self.logger.debug('Starting auto-approval of translation imports')
-        process.run()
-        self.logger.debug('Completed auto-approval of translation imports')
+from lp.translations.scripts.import_queue_gardener import ImportQueueGardener
 
 
 if __name__ == '__main__':
-    script = RosettaImportApprover('rosetta-approve-imports',
-        dbuser='poimportapprover')
-    script.lock_or_quit()
-    try:
-        script.run()
-    finally:
-        script.unlock()
-
+    script = ImportQueueGardener(
+        'translations-import-queue-gardener',
+        dbuser='translations_import_queue_gardener')
+    script.lock_and_run()
 
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2009-09-19 04:06:14 +0000
+++ database/schema/security.cfg	2009-09-26 06:25:27 +0000
@@ -441,16 +441,30 @@
 public.account                          = SELECT, INSERT
 public.customlanguagecode               = SELECT
 public.translationgroup                 = SELECT
-public.translationimportqueueentry      = SELECT, DELETE
+public.translationimportqueueentry      = SELECT
 public.translationmessage               = SELECT, INSERT, UPDATE
 public.translationrelicensingagreement  = SELECT
 public.translator                       = SELECT
 public.validpersoncache                 = SELECT
 public.validpersonorteamcache           = SELECT
 
-[poimportapprover]
+[translations_import_queue_gardener]
+# Translations import queue management
 type=user
-groups=poimport
+groups=script
+public.customlanguagecode               = SELECT
+public.distribution                     = SELECT
+public.distroseries                     = SELECT
+public.language                         = SELECT
+public.person                           = SELECT
+public.pofile                           = SELECT, INSERT, UPDATE
+public.potemplate                       = SELECT, INSERT, UPDATE
+public.product                          = SELECT
+public.productseries                    = SELECT
+public.sourcepackagename                = SELECT
+public.teamparticipation                = SELECT
+public.translationimportqueueentry      = SELECT, DELETE, UPDATE
+public.translationrelicensingagreement  = SELECT
 
 [poexport]
 # Rosetta export script
 
=== modified file 'lib/lp/translations/doc/poimport.txt'
--- lib/lp/translations/doc/poimport.txt	2009-09-03 09:10:49 +0000
+++ lib/lp/translations/doc/poimport.txt	2009-09-26 06:25:27 +0000
@@ -15,8 +15,9 @@
     ...     ITranslationImportQueue, RosettaImportStatus)
     >>> from lp.registry.model.sourcepackagename import SourcePackageName
     >>> from lp.translations.model.potemplate import POTemplateSubset
-    >>> from lp.translations.scripts.po_import import (
-    ...     AutoApproveProcess, ImportProcess)
+    >>> from lp.translations.scripts.po_import import ImportProcess
+    >>> from lp.translations.scripts.import_queue_gardener import (
+    ...     ImportQueueGardener)
     >>> import datetime
     >>> import pytz
     >>> UTC = pytz.timezone('UTC')
@@ -673,7 +674,7 @@
     <BLANKLINE>
     The Launchpad team
 
-Now the auto-approval script runs. This can happen anytime, since it's
+Now the queue gardener runs. This can happen anytime, since it's
 asynchronous to the po-import script. The script tries to approve any
 entries that have not been approved, but look like they could be,
 without human intervention. This involves a bit of guesswork about what
@@ -682,9 +683,13 @@
 entries from the queue. Running at this point, all it does is purge the
 two hand-approved Welsh translations that have just been imported.
 
-    >>> process = AutoApproveProcess(transaction, FakeLogger())
-    >>> process.run()
-    INFO    Removed 2 entries from the queue.
+    >>> import logging
+    >>> from canonical.launchpad.ftests.logger import MockLogger
+    >>> process = ImportQueueGardener('approver', test_args=[])
+    >>> process.logger = MockLogger()
+    >>> process.logger.setLevel(logging.INFO)
+    >>> process.main()
+    log>    Removed 2 entries from the queue.
     >>> transaction.commit()
 
 If users upload two versions of the same file, they are imported in the
@@ -761,13 +766,15 @@
     >>> print entry.status.name
     NEEDS_REVIEW
 
-The auto-approval script runs again. This time it sees the two
-submitted translations and approves them for import based on some
-heuristic intelligence.
+The queue gardener runs again. This time it sees the two submitted
+translations and approves them for import based on some heuristic
+intelligence.
 
-    >>> process = AutoApproveProcess(transaction, FakeLogger())
-    >>> process.run()
-    INFO    The automatic approval system approved some entries.
+    >>> process = ImportQueueGardener('approver', test_args=[])
+    >>> process.logger = MockLogger()
+    >>> process.logger.setLevel(logging.INFO)
+    >>> process.main()
+    log>    The automatic approval system approved some entries.
     >>> print entry.status.name
     APPROVED
     >>> syncUpdate(entry)
 
=== modified file 'lib/lp/translations/interfaces/translationimportqueue.py'
--- lib/lp/translations/interfaces/translationimportqueue.py	2009-08-14 16:35:06 +0000
+++ lib/lp/translations/interfaces/translationimportqueue.py	2009-09-26 06:25:27 +0000
@@ -399,30 +399,37 @@
         All returned items will implement `IHasTranslationImports`.
         """
 
-    def executeOptimisticApprovals(ztm):
-        """Try to move entries from the Needs Review status to Approved one.
+    def executeOptimisticApprovals(txn=None):
+        """Try to approve Needs-Review entries.
 
-        :arg ztm: Zope transaction manager object.
+        :arg txn: Optional transaction manager.  If given, will be
+            committed regularly.
 
         This method moves all entries that we know where should they be
         imported from the Needs Review status to the Accepted one.
         """
 
-    def executeOptimisticBlock(ztm):
+    def executeOptimisticBlock(txn=None):
         """Try to move entries from the Needs Review status to Blocked one.
 
-        :arg ztm: Zope transaction manager object or None.
-
-        This method moves all .po entries that are on the same directory that
-        a .pot entry that has the status Blocked to that same status.
-
-        Return the number of items blocked.
+        :arg txn: Optional transaction manager.  If given, will be
+            committed regularly.
+
+        This method moves uploaded translations for Blocked templates to
+        the Blocked status as well.  This lets you block a template plus
+        all its present or future translations in one go.
+
+        :return: The number of items blocked.
         """
 
     def cleanUpQueue():
-        """Remove old DELETED and IMPORTED entries.
-
-        Only entries older than 5 days will be removed.
+        """Remove old entries in terminal states.
+
+        This "garbage-collects" entries from the queue based on their
+        status (e.g. Deleted and Imported ones) and how long they have
+        been in that status.
+
+        :return: The number of entries deleted.
         """
 
     def remove(entry):
 
=== modified file 'lib/lp/translations/model/translationimportqueue.py'
--- lib/lp/translations/model/translationimportqueue.py	2009-09-03 05:14:07 +0000
+++ lib/lp/translations/model/translationimportqueue.py	2009-09-26 06:25:27 +0000
@@ -23,6 +23,7 @@
 from zope.interface import implements
 from zope.component import getUtility
 from sqlobject import SQLObjectNotFound, StringCol, ForeignKey, BoolCol
+from storm.expr import And, Or
 from storm.locals import Int, Reference
 
 from canonical.database.sqlbase import (
@@ -32,9 +33,11 @@
 from canonical.database.enumcol import EnumCol
 from canonical.launchpad.helpers import shortlist
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.launchpad.webapp.interfaces import NotFoundError
 from lp.registry.interfaces.distribution import IDistribution
-from lp.registry.interfaces.distroseries import IDistroSeries
+from lp.registry.interfaces.distroseries import (
+    IDistroSeries, DistroSeriesStatus)
 from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.productseries import IProductSeries
@@ -61,9 +64,13 @@
 from lp.registry.interfaces.person import validate_public_person
 
 
-# Number of days when the DELETED and IMPORTED entries are removed from the
-# queue.
-DAYS_TO_KEEP = 3
+# Period to wait before entries with terminal statuses are removed from
+# the queue.
+entry_gc_age = {
+    RosettaImportStatus.DELETED: datetime.timedelta(days=3),
+    RosettaImportStatus.IMPORTED: datetime.timedelta(days=3),
+    RosettaImportStatus.FAILED: datetime.timedelta(days=30),
+}
 
 
 def is_gettext_name(path):
@@ -1068,7 +1075,7 @@
 
         return distroseriess + products
 
-    def executeOptimisticApprovals(self, ztm):
+    def executeOptimisticApprovals(self, txn=None):
         """See ITranslationImportQueue."""
         there_are_entries_approved = False
         importer = getUtility(ITranslationImporter)
@@ -1108,12 +1115,13 @@
             # Already know where it should be imported. The entry is approved
             # automatically.
             entry.setStatus(RosettaImportStatus.APPROVED)
-            # Do the commit to save the changes.
-            ztm.commit()
+
+            if txn is not None:
+                txn.commit()
 
         return there_are_entries_approved
 
-    def executeOptimisticBlock(self, ztm=None):
+    def executeOptimisticBlock(self, txn=None):
         """See ITranslationImportQueue."""
         importer = getUtility(ITranslationImporter)
         num_blocked = 0
@@ -1143,40 +1151,80 @@
                 # blocked, so we can block it too.
                 entry.setStatus(RosettaImportStatus.BLOCKED)
                 num_blocked += 1
-                if ztm is not None:
-                    # Do the commit to save the changes.
-                    ztm.commit()
+                if txn is not None:
+                    txn.commit()
 
         return num_blocked
 
+    def _cleanUpObsoleteEntries(self, store):
+        """Delete obsolete queue entries.
+
+        :param store: The Store to delete from.
+        :return: Number of entries deleted.
+        """
+        now = datetime.datetime.now(pytz.UTC)
+        deletion_clauses = []
+        for status, gc_age in entry_gc_age.iteritems():
+            cutoff = now - gc_age
+            deletion_clauses.append(And(
+                TranslationImportQueueEntry.status == status,
+                TranslationImportQueueEntry.date_status_changed < cutoff))
+
+        entries = store.find(
+            TranslationImportQueueEntry, Or(*deletion_clauses))
+
+        return entries.remove()
+
+    def _cleanUpInactiveProductEntries(self, store):
+        """Delete queue entries for deactivated `Product`s.
+
+        :param store: The Store to delete from.
+        :return: Number of entries deleted.
+        """
+        # XXX JeroenVermeulen 2009-09-18 bug=271938: Stormify this once
+        # the Storm remove() syntax starts working properly for joins.
+        cur = cursor()
+        cur.execute("""
+            DELETE FROM TranslationImportQueueEntry AS Entry
+            USING ProductSeries, Product
+            WHERE
+                ProductSeries.id = Entry.productseries AND
+                Product.id = ProductSeries.product AND
+                Product.active IS FALSE
+            """)
+        return cur.rowcount
+
+    def _cleanUpObsoleteDistroEntries(self, store):
+        """Delete some queue entries for obsolete `DistroSeries`.
+
+        :param store: The Store to delete from.
+        :return: Number of entries deleted.
+        """
+        # XXX JeroenVermeulen 2009-09-18 bug=271938,432484: Stormify
+        # this once Storm's remove() supports joins and slices.
+        cur = cursor()
+        cur.execute("""
+            DELETE FROM TranslationImportQueueEntry
+            WHERE id IN (
+                SELECT Entry.id
+                FROM TranslationImportQueueEntry Entry
+                JOIN DistroSeries ON
+                    DistroSeries.id = Entry.distroseries
+                JOIN Distribution ON
+                    Distribution.id = DistroSeries.distribution
+                WHERE DistroSeries.releasestatus = %s
+                LIMIT 100)
+            """ % quote(DistroSeriesStatus.OBSOLETE))
+        return cur.rowcount
+
     def cleanUpQueue(self):
-        """See ITranslationImportQueue."""
-        cur = cursor()
-
-        # Delete outdated DELETED and IMPORTED entries.
-        delta = datetime.timedelta(DAYS_TO_KEEP)
-        last_date = datetime.datetime.utcnow() - delta
-        cur.execute("""
-            DELETE FROM TranslationImportQueueEntry
-            WHERE
-            (status = %s OR status = %s) AND date_status_changed < %s
-            """ % sqlvalues(RosettaImportStatus.DELETED.value,
-                            RosettaImportStatus.IMPORTED.value,
-                            last_date))
-        n_entries = cur.rowcount
-
-        # Delete entries belonging to inactive product series.
-        cur.execute("""
-            DELETE FROM TranslationImportQueueEntry AS entry
-            USING ProductSeries AS series, Product AS product
-            WHERE
-                entry.productseries = series.id AND
-                series.product = product.id AND
-                product.active IS FALSE
-            """)
-        n_entries += cur.rowcount
-
-        return n_entries
+        """See `ITranslationImportQueue`."""
+        store = IMasterStore(TranslationImportQueueEntry)
+
+        return (
+            self._cleanUpObsoleteEntries(store) +
+            self._cleanUpInactiveProductEntries(store) +
+            self._cleanUpObsoleteDistroEntries(store))
 
     def remove(self, entry):
         """See ITranslationImportQueue."""
 
=== added file 'lib/lp/translations/scripts/import_queue_gardener.py'
--- lib/lp/translations/scripts/import_queue_gardener.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/scripts/import_queue_gardener.py	2009-09-26 06:25:27 +0000
@@ -0,0 +1,50 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Translations auto-approval script."""
+
+__metaclass__ = type
+
+__all__ = [
+    'ImportQueueGardener',
+    ]
+
+from zope.component import getUtility
+
+from lp.services.scripts.base import LaunchpadCronScript
+from lp.translations.interfaces.translationimportqueue import (
+    ITranslationImportQueue)
+
+
+class ImportQueueGardener(LaunchpadCronScript):
+    """Automated gardening for the Translations import queue."""
+    def main(self):
+        """Manage import queue.
+
+        Approve uploads that can be approved automatically.
+        Garbage-collect ones that are no longer needed.  Block
+        translations on the queue for templates that are blocked.
+        """
+        self.logger.debug("Starting gardening of translation imports")
+
+        translation_import_queue = getUtility(ITranslationImportQueue)
+
+        if translation_import_queue.executeOptimisticApprovals(self.txn):
+            self.logger.info(
+                'The automatic approval system approved some entries.')
+
+        removed_entries = translation_import_queue.cleanUpQueue()
+        if removed_entries > 0:
+            self.logger.info('Removed %d entries from the queue.' %
+                removed_entries)
+
+        if self.txn:
+            self.txn.commit()
+
+        blocked_entries = (
+            translation_import_queue.executeOptimisticBlock(self.txn))
+        if blocked_entries > 0:
+            self.logger.info('Blocked %d entries from the queue.' %
+                blocked_entries)
+
+        self.logger.debug("Completed gardening of translation imports.")
 
=== modified file 'lib/lp/translations/scripts/po_import.py'
--- lib/lp/translations/scripts/po_import.py	2009-09-03 09:10:49 +0000
+++ lib/lp/translations/scripts/po_import.py	2009-09-26 06:25:27 +0000
@@ -6,6 +6,10 @@
 __metaclass__ = type
 
 
+__all__ = [
+    'ImportProcess',
+    ]
+
 import time
 
 from zope.component import getUtility
@@ -191,58 +195,3 @@
             self.logger.info("Import requests completed.")
         else:
             self.logger.info("Used up available time.")
-
-
-class AutoApproveProcess:
-    """Attempt to approve some PO/POT imports without human intervention."""
-    def __init__(self, ztm, logger):
-        self.ztm = ztm
-        self.logger = logger
-
-    def run(self):
-        """Attempt to approve requests without human intervention.
-
-        Look for entries in translation_import_queue that look like they can
-        be approved automatically.
-
-        Also, detect requests that should be blocked, and block them in their
-        entirety (with all their .pot and .po files); and purges completed or
-        removed entries from the queue.
-        """
-
-        translation_import_queue = getUtility(ITranslationImportQueue)
-
-        # There may be corner cases where an 'optimistic approval' could
-        # import a .po file to the wrong IPOFile (but the right language).
-        # The savings justify that risk.  The problem can only occur where,
-        # for a given productseries/sourcepackage, we have two potemplates in
-        # the same directory, each with its own set of .po files, and for some
-        # reason one of the .pot files has not been added to the queue.  Then
-        # we would import both sets of .po files to that template.  This is
-        # not a big issue because the two templates will rarely share an
-        # identical msgid, and especially because it's not a very common
-        # layout in the free software world.
-        if translation_import_queue.executeOptimisticApprovals(self.ztm):
-            self.logger.info(
-                'The automatic approval system approved some entries.')
-
-        removed_entries = translation_import_queue.cleanUpQueue()
-        if removed_entries > 0:
-            self.logger.info('Removed %d entries from the queue.' %
-                removed_entries)
-            self.ztm.commit()
-            self.ztm.begin()
-
-        # We need to block entries automatically to save Rosetta experts some
-        # work when a complete set of .po files and a .pot file should not be
-        # imported into the system.  We have the same corner case as with the
-        # previous approval method, but in this case it's a matter of changing
-        # the status back from "blocked" to "needs review," or approving it
-        # directly so no data will be lost and a lot of work is saved.
-        blocked_entries = (
-            translation_import_queue.executeOptimisticBlock(self.ztm))
-        if blocked_entries > 0:
-            self.logger.info('Blocked %d entries from the queue.' %
-                blocked_entries)
-            self.ztm.commit()
-
 
=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
--- lib/lp/translations/tests/test_autoapproval.py	2009-09-24 15:02:58 +0000
+++ lib/lp/translations/tests/test_autoapproval.py	2009-09-26 06:25:27 +0000
@@ -8,8 +8,14 @@
 through the possibilities should go here.
 """
 
+from datetime import datetime, timedelta
+from pytz import UTC
+import transaction
 import unittest
 
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
+
+from lp.registry.interfaces.distroseries import DistroSeriesStatus
 from lp.registry.model.distribution import Distribution
 from lp.registry.model.sourcepackagename import (
     SourcePackageName,
@@ -20,7 +26,7 @@
     POTemplateSet,
     POTemplateSubset)
 from lp.translations.model.translationimportqueue import (
-    TranslationImportQueue)
+    TranslationImportQueue, TranslationImportQueueEntry)
 from lp.translations.interfaces.customlanguagecode import ICustomLanguageCode
 from lp.translations.interfaces.translationimportqueue import (
     RosettaImportStatus)
@@ -30,6 +36,12 @@
 from canonical.testing import LaunchpadZopelessLayer
 
 
+def become_the_gardener(layer):
+    """Switch to the translations import queue gardener database role."""
+    transaction.commit()
+    layer.switchDbUser('translations_import_queue_gardener')
+
+
 class TestCustomLanguageCode(unittest.TestCase):
     """Unit tests for `CustomLanguageCode`."""
 
@@ -166,6 +178,7 @@
         # Of course matching will work without custom language codes.
         tr_file = self._makePOFile('tr')
         entry = self._makeQueueEntry('tr')
+        become_the_gardener(self.layer)
         self.assertEqual(entry.getGuessedPOFile(), tr_file)
 
     def test_CustomLanguageCodeEnablesMatch(self):
@@ -177,6 +190,7 @@
 
         self._setCustomLanguageCode('fy_NL', 'fy')
 
+        become_the_gardener(self.layer)
         self.assertEqual(entry.getGuessedPOFile(), fy_file)
 
     def test_CustomLanguageCodeParsesBogusLanguage(self):
@@ -187,6 +201,7 @@
 
         self._setCustomLanguageCode('flemish', 'nl')
 
+        become_the_gardener(self.layer)
         nl_file = entry.getGuessedPOFile()
         self.assertEqual(nl_file.language.code, 'nl')
 
@@ -199,6 +214,7 @@
 
         self._setCustomLanguageCode('sv', None)
 
+        become_the_gardener(self.layer)
         self.assertEqual(entry.getGuessedPOFile(), None)
         self.assertEqual(entry.status, RosettaImportStatus.DELETED)
 
@@ -211,6 +227,7 @@
 
         self._setCustomLanguageCode('elx', 'el')
 
+        become_the_gardener(self.layer)
         el_file = entry.getGuessedPOFile()
         self.failIfEqual(el_file, elx_file)
         self.assertEqual(el_file.language.code, 'el')
@@ -225,6 +242,7 @@
 
         self._setCustomLanguageCode('nb', 'nn')
 
+        become_the_gardener(self.layer)
         self.assertEqual(entry.getGuessedPOFile(), nn_file)
 
     def test_CustomLanguageCodeReplacesMatch(self):
@@ -237,6 +255,7 @@
         self._setCustomLanguageCode('pt', None)
         self._setCustomLanguageCode('pt_PT', 'pt')
 
+        become_the_gardener(self.layer)
         self.assertEqual(pt_entry.getGuessedPOFile(), None)
         self.assertEqual(pt_PT_entry.getGuessedPOFile(), pt_file)
 
@@ -250,6 +269,7 @@
         self._setCustomLanguageCode('zh_CN', 'zh_TW')
         self._setCustomLanguageCode('zh_TW', 'zh_CN')
 
+        become_the_gardener(self.layer)
         self.assertEqual(zh_CN_entry.getGuessedPOFile(), zh_TW_file)
         self.assertEqual(zh_TW_entry.getGuessedPOFile(), zh_CN_file)
 
@@ -294,6 +314,7 @@
         # When multiple templates match for a product series,
         # getPOTemplateByPathAndOrigin returns none.
         self._setUpProduct()
+        become_the_gardener(self.layer)
         guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
             'test.pot', productseries=self.productseries)
         self.assertEqual(None, guessed_template)
@@ -302,6 +323,7 @@
         # When multiple templates match on sourcepackagename,
         # getPOTemplateByPathAndOrigin returns none.
         self._setUpDistro()
+        become_the_gardener(self.layer)
         guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
             'test.pot', sourcepackagename=self.packagename)
         self.assertEqual(None, guessed_template)
@@ -310,6 +332,7 @@
         # When multiple templates match on from_sourcepackagename,
         # getPOTemplateByPathAndOrigin returns none.
         self._setUpDistro()
+        become_the_gardener(self.layer)
         guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
             'test.pot', sourcepackagename=self.from_packagename)
         self.assertEqual(None, guessed_template)
@@ -343,6 +366,7 @@
         self.distrotemplate1.sourcepackagename = match_package
         self.distrotemplate2.from_sourcepackagename = match_package
 
+        become_the_gardener(self.layer)
         guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
             'test.pot', sourcepackagename=match_package)
         self.assertEqual(self.distrotemplate2, guessed_template)
@@ -353,6 +377,7 @@
         self._setUpProduct()
         self.producttemplate1.iscurrent = False
         self.producttemplate2.iscurrent = True
+        become_the_gardener(self.layer)
         guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
             'test.pot', productseries=self.productseries)
         self.assertEqual(guessed_template, self.producttemplate2)
@@ -362,6 +387,7 @@
         self._setUpProduct()
         self.producttemplate1.iscurrent = False
         self.producttemplate2.iscurrent = False
+        become_the_gardener(self.layer)
         guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
             'test.pot', productseries=self.productseries)
         self.assertEqual(guessed_template, None)
@@ -375,6 +401,7 @@
         self.distrotemplate2.iscurrent = True
         self.distrotemplate1.from_sourcepackagename = None
         self.distrotemplate2.from_sourcepackagename = None
+        become_the_gardener(self.layer)
         guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
             'test.pot', distroseries=self.distroseries,
             sourcepackagename=self.packagename)
@@ -387,6 +414,7 @@
         self.distrotemplate2.iscurrent = False
         self.distrotemplate1.from_sourcepackagename = None
         self.distrotemplate2.from_sourcepackagename = None
+        become_the_gardener(self.layer)
         guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
             'test.pot', distroseries=self.distroseries,
             sourcepackagename=self.packagename)
@@ -401,6 +429,7 @@
         self.distrotemplate2.iscurrent = True
         self.distrotemplate1.from_sourcepackagename = self.from_packagename
         self.distrotemplate2.from_sourcepackagename = self.from_packagename
+        become_the_gardener(self.layer)
         guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
             'test.pot', distroseries=self.distroseries,
             sourcepackagename=self.from_packagename)
@@ -414,6 +443,7 @@
         self.distrotemplate2.iscurrent = False
         self.distrotemplate1.from_sourcepackagename = self.from_packagename
         self.distrotemplate2.from_sourcepackagename = self.from_packagename
+        become_the_gardener(self.layer)
         guessed_template = self.templateset.getPOTemplateByPathAndOrigin(
             'test.pot', distroseries=self.distroseries,
             sourcepackagename=self.from_packagename)
@@ -424,6 +454,7 @@
         # translation domain.
         self._setUpDistro()
         subset = POTemplateSubset(distroseries=self.distroseries)
+        become_the_gardener(self.layer)
         potemplate = subset.getPOTemplateByTranslationDomain('test1')
         self.assertEqual(potemplate, self.distrotemplate1)
 
@@ -431,6 +462,7 @@
         # Test getPOTemplateByTranslationDomain for the zero-match case.
         self._setUpDistro()
         subset = POTemplateSubset(distroseries=self.distroseries)
+        become_the_gardener(self.layer)
         potemplate = subset.getPOTemplateByTranslationDomain('notesthere')
         self.assertEqual(potemplate, None)
 
@@ -445,6 +477,7 @@
         clashing_template = other_subset.new(
             'test3', 'test1', 'test3.pot', self.distro.owner)
         distro_subset = POTemplateSubset(distroseries=self.distroseries)
+        become_the_gardener(self.layer)
         potemplate = distro_subset.getPOTemplateByTranslationDomain('test1')
         self.assertEqual(potemplate, None)
 
@@ -474,6 +507,7 @@
             'program/nl.po', 'other contents', False, template.owner,
             productseries=template.productseries, potemplate=template)
 
+        become_the_gardener(self.layer)
         entry1.getGuessedPOFile()
 
         self.assertEqual(entry1.potemplate, None)
@@ -531,6 +565,7 @@
             poname, self.pocontents, False, self.distroseries.owner,
             sourcepackagename=self.kde_i18n_ca,
             distroseries=self.distroseries)
+        become_the_gardener(self.layer)
         pofile = entry.getGuessedPOFile()
         self.assertEqual(pofile, self.pofile_ca)
 
@@ -542,6 +577,7 @@
             poname, self.pocontents, False, self.distroseries.owner,
             sourcepackagename=self.kde_l10n_nl,
             distroseries=self.distroseries)
+        become_the_gardener(self.layer)
         pofile = entry.getGuessedPOFile()
         self.assertEqual(pofile, self.pofile_nl)
 
@@ -569,6 +605,7 @@
         entry = self.queue.addOrUpdateEntry(
             'nl.po', '# ...', False, template.owner, productseries=trunk)
 
+        become_the_gardener(self.layer)
         pofile = entry._get_pofile_from_language('nl', 'domain')
         self.assertNotEqual(None, pofile)
 
@@ -586,9 +623,124 @@
         entry = self.queue.addOrUpdateEntry(
             'nl.po', '# ...', False, template.owner, productseries=trunk)
 
+        become_the_gardener(self.layer)
         pofile = entry._get_pofile_from_language('nl', 'domain')
         self.assertEqual(None, pofile)
 
 
+class TestCleanup(TestCaseWithFactory):
+    """Test `TranslationImportQueueEntry` garbage collection."""
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestCleanup, self).setUp()
+        self.queue = TranslationImportQueue()
+        self.store = IMasterStore(TranslationImportQueueEntry)
+
+    def _makeProductEntry(self):
+        """Simulate upload for a product."""
+        product = self.factory.makeProduct()
+        product.official_rosetta = True
+        trunk = product.getSeries('trunk')
+        return self.queue.addOrUpdateEntry(
+            'foo.pot', '# contents', False, product.owner,
+            productseries=trunk)
+
+    def _makeDistroEntry(self):
+        """Simulate upload for a distribution package."""
+        package = self.factory.makeSourcePackage()
+        owner = package.distroseries.owner
+        return self.queue.addOrUpdateEntry(
+            'bar.pot', '# contents', False, owner,
+            sourcepackagename=package.sourcepackagename,
+            distroseries=package.distroseries)
+
+    def _exists(self, entry_id):
+        """Is the entry with the given id still on the queue?"""
+        entry = self.store.find(
+            TranslationImportQueueEntry,
+            TranslationImportQueueEntry.id == entry_id).any()
+        return entry is not None
+
+    def _setStatus(self, entry, status, when=None):
+        """Simulate status on queue entry having been set at a given time."""
+        entry.setStatus(status)
+        if when is not None:
+            entry.date_status_changed = when
+        entry.syncUpdate()
+
+    def test_cleanUpObsoleteEntries_unaffected_statuses(self):
+        # _cleanUpObsoleteEntries leaves entries in non-terminal states
+        # (Needs Review, Approved, Blocked) alone no matter how old they
+        # are.
+        one_year_ago = datetime.now(UTC) - timedelta(days=366)
+        entry = self._makeProductEntry()
+        entry_id = entry.id
+
+        self._setStatus(entry, RosettaImportStatus.APPROVED, one_year_ago)
+        self.queue._cleanUpObsoleteEntries(self.store)
+        self.assertTrue(self._exists(entry_id))
+
+        self._setStatus(entry, RosettaImportStatus.BLOCKED, one_year_ago)
+        self.queue._cleanUpObsoleteEntries(self.store)
+        self.assertTrue(self._exists(entry_id))
+
+        self._setStatus(entry, RosettaImportStatus.NEEDS_REVIEW, one_year_ago)
+        become_the_gardener(self.layer)
+        self.queue._cleanUpObsoleteEntries(self.store)
+        self.assertTrue(self._exists(entry_id))
+
+    def test_cleanUpObsoleteEntries_affected_statuses(self):
+        # _cleanUpObsoleteEntries deletes entries in terminal states
+        # (Imported, Failed, Deleted) after a few days.  The exact
+        # period depends on the state.
+        entry = self._makeProductEntry()
+        self._setStatus(entry, RosettaImportStatus.IMPORTED, None)
+        entry_id = entry.id
+
+        self.queue._cleanUpObsoleteEntries(self.store)
+        self.assertTrue(self._exists(entry_id))
+
+        entry.date_status_changed -= timedelta(days=7)
+        entry.syncUpdate()
+
+        become_the_gardener(self.layer)
+        self.queue._cleanUpObsoleteEntries(self.store)
+        self.assertFalse(self._exists(entry_id))
+
+    def test_cleanUpInactiveProductEntries(self):
+        # After a product is deactivated, _cleanUpInactiveProductEntries
+        # will clean up any entries it may have on the queue.
+        entry = self._makeProductEntry()
+        entry_id = entry.id
+
+        self.queue._cleanUpInactiveProductEntries(self.store)
+        self.assertTrue(self._exists(entry_id))
+
+        entry.productseries.product.active = False
+        entry.productseries.product.syncUpdate()
+
+        become_the_gardener(self.layer)
+        self.queue._cleanUpInactiveProductEntries(self.store)
+        self.assertFalse(self._exists(entry_id))
+
+    def test_cleanUpObsoleteDistroEntries(self):
+        # _cleanUpObsoleteDistroEntries cleans up entries for
+        # distroseries that are in the Obsolete state.
+        entry = self._makeDistroEntry()
+        entry_id = entry.id
+
+        self.queue._cleanUpObsoleteDistroEntries(self.store)
+        self.assertTrue(self._exists(entry_id))
+
+        entry.distroseries.status = DistroSeriesStatus.OBSOLETE
+        entry.distroseries.syncUpdate()
+
+        become_the_gardener(self.layer)
+        self.queue._cleanUpObsoleteDistroEntries(self.store)
+        self.assertFalse(self._exists(entry_id))
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)