Merge lp:~benji/launchpad/bug-877195-code into lp:launchpad

[Benji York]

[r=jcsackett] move POFile statistics update into a job

When a change is made to a translation several statistics about that
translation are updated. That update is slow. So slow that it often
times out. Bug 877195 is about moving those statistics updates into a
job so the web requests can return faster but the statistics will still
be relatively up to date.

I had pre-implementation calls with Danilo.

There is a prerequisite branch that includes the database changes.

Tests for the new job are in
lib/lp/translations/tests/test_pofilestatsjob.py and can be run like so:

    bin/test -c -m lp.translations.tests.test_pofilestatsjob

The "make lint" report is clean.

QA: note a translations statistics, make a change to a translation that
would impact its statistics, get a LOSA to run the cron job, verify that
the statistics changed.

[j.c.sackett (jcsackett)]

This looks good to me, Benji. Thanks for offloading this work into a job, sounds like a good fix.

review: Approve

=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf	2011-11-14 06:04:41 +0000
+++ lib/canonical/config/schema-lazr.conf	2011-11-22 05:26:23 +0000
@@ -1855,6 +1855,13 @@
 dbuser: send-branch-mail
 storm_cache: generational
 storm_cache_size: 500
+oops_prefix: none
+error_dir: none
+
+[pofile_stats]
+dbuser: pofilestats
+storm_cache: generational
+source_interface: lp.translations.interfaces.pofilestatsjob.IPOFileStatsJobSource
 
 # See [error_reports].
 error_dir: none

=== modified file 'lib/lp/translations/browser/translationmessage.py'
--- lib/lp/translations/browser/translationmessage.py	2011-07-13 06:08:16 +0000
+++ lib/lp/translations/browser/translationmessage.py	2011-11-22 05:26:23 +0000
@@ -70,6 +70,7 @@
     )
 from lp.translations.interfaces.translations import TranslationConstants
 from lp.translations.interfaces.translationsperson import ITranslationsPerson
+from lp.translations.model import pofilestatsjob
 from lp.translations.utilities.sanitize import (
     sanitize_translations_from_webui,
     )
@@ -892,10 +893,8 @@
 
     def _redirectToNextPage(self):
         """After a successful submission, redirect to the next batch page."""
-        # XXX: kiko 2006-09-27:
-        # Isn't this a hell of a performance issue, hitting this
-        # same table for every submit?
-        self.pofile.updateStatistics()
+        # Schedule this POFile to have its statistics updated.
+        pofilestatsjob.schedule(self.pofile)
         next_url = self.batchnav.nextBatchURL()
         if next_url is None or next_url == '':
             # We are already at the end of the batch, forward to the

=== modified file 'lib/lp/translations/configure.zcml'
--- lib/lp/translations/configure.zcml	2011-10-18 10:09:58 +0000
+++ lib/lp/translations/configure.zcml	2011-11-22 05:26:23 +0000
@@ -694,4 +694,13 @@
 
   <webservice:register module="lp.translations.interfaces.webservice" />
 
+  <class
+      class="lp.translations.model.pofilestatsjob.POFileStatsJob">
+      <allow interface='lp.services.job.interfaces.job.IRunnableJob'/>
+  </class>
+  <utility
+      component="lp.translations.model.pofilestatsjob.POFileStatsJob"
+      provides="lp.translations.interfaces.pofilestatsjob.IPOFileStatsJobSource"
+      />
+
 </configure>

=== added file 'lib/lp/translations/interfaces/pofilestatsjob.py'
--- lib/lp/translations/interfaces/pofilestatsjob.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/interfaces/pofilestatsjob.py	2011-11-22 05:26:23 +0000
@@ -0,0 +1,16 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+# pylint: disable-msg=E0213
+
+__metaclass__ = type
+
+__all__ = [
+    'IPOFileStatsJobSource',
+    ]
+
+from lp.services.job.interfaces.job import IJobSource
+
+
+class IPOFileStatsJobSource(IJobSource):
+    """A source for jobs to update POFile statistics."""

=== added file 'lib/lp/translations/model/pofilestatsjob.py'
--- lib/lp/translations/model/pofilestatsjob.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/model/pofilestatsjob.py	2011-11-22 05:26:23 +0000
@@ -0,0 +1,88 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Job for merging translations."""
+
+
+__metaclass__ = type
+
+
+__all__ = [
+    'POFileStatsJob',
+    ]
+
+import logging
+from zope.component import getUtility
+
+from storm.locals import (
+    And,
+    Int,
+    Reference,
+    )
+from zope.interface import (
+    classProvides,
+    implements,
+    )
+
+from canonical.launchpad.webapp.interfaces import (
+    DEFAULT_FLAVOR,
+    IStoreSelector,
+    MAIN_STORE,
+    )
+from lp.services.database.stormbase import StormBase
+from lp.services.job.interfaces.job import IRunnableJob
+from lp.services.job.model.job import Job
+from lp.services.job.runner import BaseRunnableJob
+from lp.translations.interfaces.pofilestatsjob import IPOFileStatsJobSource
+from lp.translations.model.pofile import POFile
+
+
+class POFileStatsJob(StormBase, BaseRunnableJob):
+    """The details for a POFile status update job."""
+
+    __storm_table__ = 'POFileStatsJob'
+
+    # Instances of this class are runnable jobs.
+    implements(IRunnableJob)
+
+    # Oddly, BaseRunnableJob inherits from BaseRunnableJobSource so this class
+    # is both the factory for jobs (the "implements", above) and the source
+    # for runnable jobs (not the constructor of the job source, the class
+    # provides the IJobSource interface itself).
+    classProvides(IPOFileStatsJobSource)
+
+    # The Job table contains core job details.
+    job_id = Int('job', primary=True)
+    job = Reference(job_id, Job.id)
+
+    # This is the POFile which needs its statistics updated.
+    pofile_id = Int('pofile')
+    pofile = Reference(pofile_id, POFile.id)
+
+    def __init__(self, pofile):
+        self.job = Job()
+        self.pofile = pofile
+        super(POFileStatsJob, self).__init__()
+
+    def getOperationDescription(self):
+        """See `IRunnableJob`."""
+        return 'updating POFile statistics'
+
+    def run(self):
+        """See `IRunnableJob`."""
+        logger = logging.getLogger()
+        logger.info('Updating statistics for %s' % self.pofile.title)
+        self.pofile.updateStatistics()
+
+    @staticmethod
+    def iterReady():
+        """See `IJobSource`."""
+        store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+        return store.find((POFileStatsJob),
+            And(POFileStatsJob.job == Job.id,
+                Job.id.is_in(Job.ready_jobs)))
+
+
+def schedule(pofile):
+    """Schedule a job to update a POFile's stats."""
+    return POFileStatsJob(pofile)

=== added file 'lib/lp/translations/tests/test_pofilestatsjob.py'
--- lib/lp/translations/tests/test_pofilestatsjob.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/tests/test_pofilestatsjob.py	2011-11-22 05:26:23 +0000
@@ -0,0 +1,74 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for merging translations."""
+
+__metaclass__ = type
+
+
+from canonical.launchpad.webapp.testing import verifyObject
+from canonical.testing.layers import (
+    LaunchpadZopelessLayer,
+    )
+from lp.services.job.interfaces.job import (
+    IJobSource,
+    IRunnableJob,
+    )
+from lp.testing import TestCaseWithFactory
+from lp.translations.interfaces.pofilestatsjob import IPOFileStatsJobSource
+from lp.translations.interfaces.side import TranslationSide
+from lp.translations.model import pofilestatsjob
+from lp.translations.model.pofilestatsjob import POFileStatsJob
+
+
+class TestPOFileStatsJob(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def test_job_interface(self):
+        # Instances of POFileStatsJob are runnable jobs.
+        verifyObject(IRunnableJob, POFileStatsJob(0))
+
+    def test_source_interface(self):
+        # The POFileStatsJob class is a source of POFileStatsJobs.
+        verifyObject(IPOFileStatsJobSource, POFileStatsJob)
+        verifyObject(IJobSource, POFileStatsJob)
+
+    def test_run(self):
+        # Running a job causes the POFile statistics to be updated.
+        singular = self.factory.getUniqueString()
+        pofile = self.factory.makePOFile(side=TranslationSide.UPSTREAM)
+        # Create a message so we have something to have statistics about.
+        self.factory.makePOTMsgSet(pofile.potemplate, singular)
+        # The statistics start at 0.
+        self.assertEqual(pofile.potemplate.messageCount(), 0)
+        job = pofilestatsjob.schedule(pofile.id)
+        # Just scheduling the job doesn't update the statistics.
+        self.assertEqual(pofile.potemplate.messageCount(), 0)
+        job.run()
+        # Now that the job ran, the statistics have been updated.
+        self.assertEqual(pofile.potemplate.messageCount(), 1)
+
+    def test_iterReady(self):
+        # The POFileStatsJob class provides a way to iterate over the jobs
+        # that are ready to run.  Initially, there aren't any.
+        self.assertEqual(len(list(POFileStatsJob.iterReady())), 0)
+        # We need a POFile to update.
+        pofile = self.factory.makePOFile(side=TranslationSide.UPSTREAM)
+        # If we schedule a job, then we'll get it back.
+        job = pofilestatsjob.schedule(pofile.id)
+        self.assertIs(list(POFileStatsJob.iterReady())[0], job)
+
+    def test_second_job_is_scheduled(self):
+        # If there is already one POFileStatsJob scheduled for a particular
+        # POFile, then a second one is scheduled.
+        self.assertEqual(len(list(POFileStatsJob.iterReady())), 0)
+        # We need a POFile to update.
+        pofile = self.factory.makePOFile(side=TranslationSide.UPSTREAM)
+        # If we schedule a job, then there will be one scheduled.
+        pofilestatsjob.schedule(pofile.id)
+        self.assertIs(len(list(POFileStatsJob.iterReady())), 1)
+        # If we attempt to schedule another job for the same POFile, a new job
+        # is added.
+        pofilestatsjob.schedule(pofile.id)
+        self.assertIs(len(list(POFileStatsJob.iterReady())), 2)