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

[Jeroen T. Vermeulen]

Scalability fix in translations import queue gardener.

= Bug 181254 =

This addresses a scalability problem in the translations import queue gardener. One of the things it does is go over all entries that need review to see which should be automatically blocked. For each entry that needs review (about 20K—30K per run during normal operation but potentially a lot more) it queries the blocked entries for ones representing templates for the same target, and usually finds nothing. This took up a significant chunk of the script's runtime in a test I ran ages ago.

In this branch I make those nested-loop queries unnecessary, fetching all the information at once instead. It's currently only 2640 tuples containing a few ints and a short string.

To test:
{{{
./bin/test -vv -m lp.translations -t autoapproval
}}}

No lint.

For Q/A, we can check that the number of blocked entries in the staging database does not jump. At the moment there are 70662, of which 2640 are templates. We can also verify that blocking still works.

Jeroen

[Brad Crittenden (bac)]

Hi Jeroen,

Other than the lint issues and the out-of-date comment that we discussed on IRC this branch looks good to me. Thanks.

review: Approve (code)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-07-01 15:22:59 +0000
+++ lib/lp/testing/factory.py	2010-07-02 14:56:31 +0000
@@ -147,12 +147,16 @@
 from lp.testing import run_with_login, time_counter, login, logout, temp_dir
 
 from lp.translations.interfaces.potemplate import IPOTemplateSet
+from lp.translations.interfaces.translationimportqueue import (
+    RosettaImportStatus)
 from lp.translations.interfaces.translationgroup import (
     ITranslationGroupSet)
 from lp.translations.interfaces.translationsperson import ITranslationsPerson
-from lp.translations.interfaces.translator import ITranslatorSet
 from lp.translations.interfaces.translationtemplatesbuildjob import (
     ITranslationTemplatesBuildJobSource)
+from lp.translations.interfaces.translator import ITranslatorSet
+from lp.translations.model.translationimportqueue import (
+    TranslationImportQueueEntry)
 
 
 SPACE = ' '
@@ -2033,6 +2037,42 @@
         translation.is_imported = is_imported
         translation.is_current = True
 
+    def makeTranslationImportQueueEntry(self, path=None, status=None,
+                                        sourcepackagename=None,
+                                        distroseries=None,
+                                        productseries=None, content=None,
+                                        uploader=None, is_published=False):
+        """Create a `TranslationImportQueueEntry`."""
+        if path is None:
+            path = self.getUniqueString() + '.pot'
+        if status is None:
+            status = RosettaImportStatus.NEEDS_REVIEW
+
+        if (sourcepackagename is None and distroseries is None):
+            if productseries is None:
+                productseries = self.makeProductSeries()
+        else:
+            if sourcepackagename is None:
+                sourcepackagename = self.makeSourcePackageName()
+            if distroseries is None:
+                distroseries = self.makeDistroSeries()
+
+        if uploader is None:
+            uploader = self.makePerson()
+
+        if content is None:
+            content = self.getUniqueString()
+
+        content_reference = getUtility(ILibraryFileAliasSet).create(
+            name=os.path.basename(path), size=len(content),
+            file=StringIO(content), contentType='text/plain')
+
+        return TranslationImportQueueEntry(
+            path=path, status=status, sourcepackagename=sourcepackagename,
+            distroseries=distroseries, productseries=productseries,
+            importer=uploader, content=content_reference,
+            is_published=is_published)
+
     def makeMailingList(self, team, owner):
         """Create a mailing list for the team."""
         team_list = getUtility(IMailingListSet).new(team, owner)

=== modified file 'lib/lp/translations/doc/translationimportqueue.txt'
--- lib/lp/translations/doc/translationimportqueue.txt	2010-04-21 16:15:36 +0000
+++ lib/lp/translations/doc/translationimportqueue.txt	2010-07-02 14:56:31 +0000
@@ -785,6 +785,7 @@
 We need it blocked for this test.
 
     >>> entry5.setStatus(RosettaImportStatus.BLOCKED, rosetta_experts)
+    >>> transaction.commit()
 
 Let's see how many entries are blocked.
 

=== modified file 'lib/lp/translations/model/translationimportqueue.py'
--- lib/lp/translations/model/translationimportqueue.py	2010-04-29 20:41:08 +0000
+++ lib/lp/translations/model/translationimportqueue.py	2010-07-02 14:56:31 +0000
@@ -34,7 +34,7 @@
 from canonical.launchpad.helpers import shortlist
 from canonical.launchpad.interfaces.launchpad import (
     ILaunchpadCelebrities, IPersonRoles)
-from canonical.launchpad.interfaces.lpstorm import IMasterStore
+from canonical.launchpad.interfaces.lpstorm import IMasterStore, ISlaveStore
 from canonical.launchpad.webapp.interfaces import NotFoundError
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.series import SeriesStatus
@@ -1204,32 +1204,84 @@
 
         return there_are_entries_approved
 
+    def _getSlaveStore(self):
+        """Return the slave store for the import queue.
+
+        Tests can override this to avoid unnecessary synchronization
+        issues.
+        """
+        return ISlaveStore(TranslationImportQueueEntry)
+
+    def _getBlockableDirectories(self):
+        """Describe all directories where uploads are to be blocked.
+
+        Returns a set of tuples, each containing:
+         * `DistroSeries` id
+         * `SourcePackageName` id
+         * `ProductSeries` id
+         * Directory path.
+
+        A `TranslationImportQueueEntry` should be blocked if the tuple
+        of its distroseries.id, sourcepackagename.id, productseries.id,
+        and the directory component of its path is found in the result
+        set.
+
+        See `_isBlockable`, which matches a queue entry against the set
+        returned by this method.
+        """
+        importer = getUtility(ITranslationImporter)
+        template_patterns = "(%s)" % ' OR '.join([
+            "path LIKE ('%%' || %s)" % quote_like(suffix)
+            for suffix in importer.template_suffixes
+            ])
+
+        store = self._getSlaveStore()
+        result = store.execute("""
+            SELECT
+                distroseries,
+                sourcepackagename,
+                productseries,
+                regexp_replace(
+                    regexp_replace(path, '^[^/]*$', ''),
+                    '/[^/]*$',
+                    '') AS directory
+            FROM TranslationImportQueueEntry
+            WHERE %(is_template)s
+            GROUP BY distroseries, sourcepackagename, productseries, directory
+            HAVING bool_and(status = %(blocked)s)
+            ORDER BY distroseries, sourcepackagename, productseries, directory
+            """ % {
+                'blocked': quote(RosettaImportStatus.BLOCKED),
+                'is_template': template_patterns,
+            })
+
+        return set(result)
+
+    def _isBlockable(self, entry, blocklist):
+        """Is `entry` one that should be blocked according to `blocklist`?
+
+        :param entry: A `TranslationImportQueueEntry` that may be a
+            candidate for blocking.
+        :param blocklist: A description of blockable directories as
+            returned by `_getBlockableDirectories`.
+        """
+        description = (
+            entry.distroseries_id,
+            entry.sourcepackagename_id,
+            entry.productseries_id,
+            os.path.dirname(entry.path),
+            )
+        return description in blocklist
+
     def executeOptimisticBlock(self, txn=None):
         """See ITranslationImportQueue."""
-        importer = getUtility(ITranslationImporter)
+        # Find entries where all template entries for the same
+        # translation target that are in the same directory are in the
+        # Blocked state.  Set those entries to Blocked as well.
+        blocklist = self._getBlockableDirectories()
         num_blocked = 0
         for entry in self._iterNeedsReview():
-            if importer.isTemplateName(entry.path):
-                # Templates cannot be managed automatically.  Ignore them and
-                # wait for an admin to do it.
-                continue
-            # As kiko would say... this method is crack, I know it, but we
-            # need it to save time to our poor Rosetta Experts while handling
-            # the translation import queue...
-            # We need to look for all templates that we have on the same
-            # directory for the entry we are processing, and check that all of
-            # them are blocked. If there is at least one that's not blocked,
-            # we cannot block the entry.
-            templates = entry.getTemplatesOnSameDirectory()
-            has_templates = False
-            has_templates_unblocked = False
-            for template in templates:
-                has_templates = True
-                if template.status != RosettaImportStatus.BLOCKED:
-                    # This template is not set as blocked, so we note it.
-                    has_templates_unblocked = True
-
-            if has_templates and not has_templates_unblocked:
+            if self._isBlockable(entry, blocklist):
                 # All templates on the same directory as this entry are
                 # blocked, so we can block it too.
                 entry.setStatus(

=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
--- lib/lp/translations/tests/test_autoapproval.py	2010-06-07 10:24:03 +0000
+++ lib/lp/translations/tests/test_autoapproval.py	2010-07-02 14:56:31 +0000
@@ -25,6 +25,7 @@
 from lp.registry.model.sourcepackagename import (
     SourcePackageName,
     SourcePackageNameSet)
+from lp.testing.fakemethod import FakeMethod
 from lp.services.worlddata.model.language import Language, LanguageSet
 from lp.translations.model.customlanguagecode import CustomLanguageCode
 from lp.translations.model.pofile import POFile
@@ -354,8 +355,7 @@
         self._setUpDistro()
         other_series = self.factory.makeDistroRelease(
             distribution=self.distro)
-        other_template = self._makeTemplateForDistroSeries(
-            other_series, 'test1')
+        self._makeTemplateForDistroSeries(other_series, 'test1')
         self.distrotemplate1.iscurrent = False
         self.distrotemplate2.iscurrent = True
         self.distrotemplate1.from_sourcepackagename = None
@@ -514,7 +514,7 @@
 
         # The clashing entry goes through approval unsuccessfully, but
         # without causing breakage.
-        entry2 = queue.addOrUpdateEntry(
+        queue.addOrUpdateEntry(
             'program/nl.po', 'other contents', False, template.owner,
             productseries=template.productseries, potemplate=template)
 
@@ -712,8 +712,7 @@
         template = self.factory.makePOTemplate(
             productseries=trunk, translation_domain='domain')
         template.iscurrent = True
-        credits = self.factory.makePOTMsgSet(template, "translator-credits",
-                                             sequence=1)
+        self.factory.makePOTMsgSet(template, "translator-credits", sequence=1)
 
         entry = self.queue.addOrUpdateEntry(
             'nl.po', '# ...', False, template.owner, productseries=trunk)
@@ -825,7 +824,6 @@
                     self._exists(entry_id),
                     "Queue entry in state '%s' was not removed." % status)
 
-
     def test_cleanUpInactiveProductEntries(self):
         # After a product is deactivated, _cleanUpInactiveProductEntries
         # will clean up any entries it may have on the queue.
@@ -886,7 +884,7 @@
         # gardener has permissions to do this.  The POFile's owner is
         # the rosetta_experts team.
         trunk = self.product.getSeries('trunk')
-        template = self._makeTemplate(trunk)
+        self._makeTemplate(trunk)
         entry = self._makeQueueEntry(trunk)
         rosetta_experts = getUtility(ILaunchpadCelebrities).rosetta_experts
 
@@ -910,11 +908,249 @@
 
         self.becomeTheGardener()
 
-        pofile = entry.getGuessedPOFile()
+        entry.getGuessedPOFile()
 
         credits.getCurrentTranslationMessage(template, self.language)
         self.assertNotEqual(None, credits)
 
 
+class TestAutoBlocking(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestAutoBlocking, self).setUp()
+        self.queue = TranslationImportQueue()
+        # Our test queue operates on the master store instead of the
+        # slave store so we don't have to synchronize stores.
+        master_store = IMasterStore(TranslationImportQueueEntry)
+        self.queue._getSlaveStore = FakeMethod(result=master_store)
+
+    def _copyTargetFromEntry(self, entry):
+        """Return a dict representing `entry`'s translation target.
+
+        :param entry: An existing `TranslationImportQueueEntry`, or None.
+        """
+        if entry is None:
+            return {}
+        else:
+            return {
+                'distroseries': entry.distroseries,
+                'sourcepackagename': entry.sourcepackagename,
+                'productseries': entry.productseries,
+            }
+
+    def _makeTemplateEntry(self, suffix='.pot', directory=None, status=None,
+                           same_target_as=None):
+        """Create an import queue entry for a template.
+
+        If `same_target_as` is given, creates an entry for the same
+        translation target as `same_target_as`.  This lets you create an
+        entry for the same translation target as another one.
+        """
+        if suffix == '.xpi':
+            basename = 'en-US'
+        else:
+            basename = self.factory.getUniqueString()
+
+        filename = basename + suffix
+        if directory is None:
+            path = filename
+        else:
+            path = '/'.join([directory, filename])
+
+        target = self._copyTargetFromEntry(same_target_as)
+
+        return self.factory.makeTranslationImportQueueEntry(
+            path=path, status=status, **target)
+
+    def _makeTranslationEntry(self, path, status=None, same_target_as=None):
+        """Create an import queue entry for a translation file.
+
+        If `same_target_as` is given, creates an entry for the same
+        translation target as `same_target_as`.  This lets you create an
+        entry that may have to be blocked depending on same_target_as.
+        """
+        target = self._copyTargetFromEntry(same_target_as)
+        return self.factory.makeTranslationImportQueueEntry(
+            path=path, status=status, **target)
+
+    def test_getBlockableDirectories_checks_templates(self):
+        old_blocklist = self.queue._getBlockableDirectories()
+
+        self._makeTemplateEntry(status=RosettaImportStatus.BLOCKED)
+
+        new_blocklist = self.queue._getBlockableDirectories()
+
+        self.assertEqual(len(old_blocklist) + 1, len(new_blocklist))
+
+    def test_getBlockableDirectories_ignores_translations(self):
+        old_blocklist = self.queue._getBlockableDirectories()
+
+        self._makeTranslationEntry(
+            'gl.po', status=RosettaImportStatus.BLOCKED)
+
+        new_blocklist = self.queue._getBlockableDirectories()
+
+        self.assertEqual(len(old_blocklist), len(new_blocklist))
+
+    def test_getBlockableDirectories_checks_xpi_templates(self):
+        old_blocklist = self.queue._getBlockableDirectories()
+
+        self._makeTemplateEntry(
+            suffix='.xpi', status=RosettaImportStatus.BLOCKED)
+
+        new_blocklist = self.queue._getBlockableDirectories()
+
+        self.assertEqual(len(old_blocklist) + 1, len(new_blocklist))
+
+    def test_getBlockableDirectories_ignores_xpi_translations(self):
+        old_blocklist = self.queue._getBlockableDirectories()
+
+        self._makeTranslationEntry(
+            'lt.xpi', status=RosettaImportStatus.BLOCKED)
+
+        new_blocklist = self.queue._getBlockableDirectories()
+
+        self.assertEqual(len(old_blocklist), len(new_blocklist))
+
+    def test_isBlockable_none(self):
+        blocklist = self.queue._getBlockableDirectories()
+        entry = self._makeTranslationEntry('nl.po')
+        self.assertFalse(self.queue._isBlockable(entry, blocklist))
+
+    def test_isBlockable_one_blocked(self):
+        blocked_template = self._makeTemplateEntry(
+            status=RosettaImportStatus.BLOCKED)
+        blocklist = self.queue._getBlockableDirectories()
+
+        translations = self._makeTranslationEntry(
+            'de.po', same_target_as=blocked_template)
+
+        self.assertTrue(self.queue._isBlockable(translations, blocklist))
+
+    def test_isBlockable_multiple_blocked(self):
+        blocked1 = self._makeTemplateEntry(status=RosettaImportStatus.BLOCKED)
+        self._makeTemplateEntry(
+            status=RosettaImportStatus.BLOCKED, same_target_as=blocked1)
+        blocklist = self.queue._getBlockableDirectories()
+
+        translations = self._makeTranslationEntry(
+            'lo.po', same_target_as=blocked1)
+
+        self.assertTrue(self.queue._isBlockable(translations, blocklist))
+
+    def test_isBlockable_one_unblocked(self):
+        unblocked = self._makeTemplateEntry()
+        blocklist = self.queue._getBlockableDirectories()
+
+        translations = self._makeTranslationEntry(
+            'xh.po', same_target_as=unblocked)
+
+        self.assertFalse(self.queue._isBlockable(translations, blocklist))
+
+    def test_isBlockable_mixed(self):
+        # When there are both blocked and unblocked template entries in
+        # a directory, translation uploads for that directory are not
+        # blocked.
+        blocked = self._makeTemplateEntry(status=RosettaImportStatus.BLOCKED)
+        self._makeTemplateEntry(same_target_as=blocked)
+        blocklist = self.queue._getBlockableDirectories()
+
+        translations = self._makeTranslationEntry(
+            'fr.po', same_target_as=blocked)
+
+        self.assertFalse(self.queue._isBlockable(translations, blocklist))
+
+    def test_getBlockableDirectories_path_rootdir_match(self):
+        # _getBlockableDirectories matches sees a template and
+        # translations file in the root directory as being in the same
+        # directory.
+        blocked = self._makeTemplateEntry(
+            directory=None, status=RosettaImportStatus.BLOCKED)
+        blocklist = self.queue._getBlockableDirectories()
+        translations = self._makeTranslationEntry(
+            'es.po', same_target_as=blocked)
+        self.assertTrue(self.queue._isBlockable(translations, blocklist))
+
+    def test_getBlockableDirectories_path_rootdir_nonmatch(self):
+        # _getBlockableDirectories matches sees a template in the root
+        # directory (i.e. without a directory component in its path) as
+        # being in a different directory from a translations upload in a
+        # subdirectory.
+        blocked = self._makeTemplateEntry(
+            directory=None, status=RosettaImportStatus.BLOCKED)
+        blocklist = self.queue._getBlockableDirectories()
+        translations = self._makeTranslationEntry(
+            'po/es.po', same_target_as=blocked)
+        self.assertFalse(self.queue._isBlockable(translations, blocklist))
+
+    def test_getBlockableDirectories_path_subdir_match(self):
+        # _getBlockableDirectories matches sees a template and
+        # translations file in the same directory as being in the same
+        # directory.
+        blocked = self._makeTemplateEntry(
+            directory='po/module', status=RosettaImportStatus.BLOCKED)
+        blocklist = self.queue._getBlockableDirectories()
+        translations = self._makeTranslationEntry(
+            'po/module/es.po', same_target_as=blocked)
+        self.assertTrue(self.queue._isBlockable(translations, blocklist))
+
+    def test_getBlockableDirectories_path_subdir_nonmatch(self):
+        # _getBlockableDirectories matches sees a template in a
+        # subdirectory as being in a different directory from a
+        # translations upload in the root directory.
+        blocked = self._makeTemplateEntry(
+            directory='po', status=RosettaImportStatus.BLOCKED)
+        blocklist = self.queue._getBlockableDirectories()
+        translations = self._makeTranslationEntry(
+            'es.po', same_target_as=blocked)
+        self.assertFalse(self.queue._isBlockable(translations, blocklist))
+
+    def test_getBlockableDirectories_path_nested_translations(self):
+        # _getBlockableDirectories sees a translations upload in a
+        # subdirectory of that on the template upload as being in a
+        # different directory.
+        blocked = self._makeTemplateEntry(
+            directory='po', status=RosettaImportStatus.BLOCKED)
+        blocklist = self.queue._getBlockableDirectories()
+        translations = self._makeTranslationEntry(
+            'po/module/es.po', same_target_as=blocked)
+        self.assertFalse(self.queue._isBlockable(translations, blocklist))
+
+    def test_getBlockableDirectories_path_nested_template(self):
+        # _getBlockableDirectories sees a translations upload in one
+        # directory and a template upload in a subdirectory of that
+        # directory as being in different directories.
+        blocked = self._makeTemplateEntry(
+            directory='po/module', status=RosettaImportStatus.BLOCKED)
+        blocklist = self.queue._getBlockableDirectories()
+        translations = self._makeTranslationEntry(
+            'po/es.po', same_target_as=blocked)
+        self.assertFalse(self.queue._isBlockable(translations, blocklist))
+
+    def test_getBlockableDirectories_path_substring_translations(self):
+        # _getBlockableDirectories sees the difference between a
+        # template's directory and a translation upload's directory even
+        # if the latter is a substring of the former.
+        blocked = self._makeTemplateEntry(
+            directory='po/moduleX', status=RosettaImportStatus.BLOCKED)
+        blocklist = self.queue._getBlockableDirectories()
+        translations = self._makeTranslationEntry(
+            'po/module/es.po', same_target_as=blocked)
+        self.assertFalse(self.queue._isBlockable(translations, blocklist))
+
+    def test_getBlockableDirectories_path_substring_template(self):
+        # _getBlockableDirectories sees the difference between a
+        # template's directory and a translation upload's directory even
+        # if the former is a substring of the latter.
+        blocked = self._makeTemplateEntry(
+            directory='po/module', status=RosettaImportStatus.BLOCKED)
+        blocklist = self.queue._getBlockableDirectories()
+        translations = self._makeTranslationEntry(
+            'po/moduleX/es.po', same_target_as=blocked)
+        self.assertFalse(self.queue._isBlockable(translations, blocklist))
+
+
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)