Merge lp:~jtv/launchpad/bug-994650-scrub-pofiletranslator into lp:launchpad

[Jeroen T. Vermeulen]

POFileTranslator scrubber script.

This is as discussed on the internal mailing list. In a series of branches that simplifies and streamlines POFileTranslator management, this is the only one that introduces new code. It won't run while we still have POFileTranslator.latest_message, which I'm dropping in a separate pair of branches (devel/db-devel).

POFileTranslator is described as an “eager materialized view.” It's a cache of who contributed TranslationMessages to which POFiles, and when they last did.

What you see in this branch is a scrubbing script for POFileTranslator. Instead of trying (and in many ways failing) to maintain that table synchronously using a long and complex database trigger, Launchpad will keep it “eventually consistent” by running this script. The script doesn't care how data got where it is, so some painfully complex script code that updates the table can disappear. Instead, the script just finds out which entries are missing and which ones need adding, and does that.

The script may still need optimizing. I deliberately kept it simple. In the current code it would have been easy to check for and correct timestamps on the contributions as well, but I think that's of secondary importantance. Where we have a justified POFileTranslator record, its timestamp is likely to be correct; the known problems involve records being omitted altogether. And simplifications I'm introducing elsewhere may also cause records to be omitted, or left in place when they should be moved to different POFiles, or deleted.

[Benji York (benji)]

This branch looks good. Here are a couple of things I spotted
while reading over it.

I believe that this bit is a syntax error in Python 2.6 as dict
comprehensions were added in 2.7. (line 132 of the diff, from
scrub_pofiletranslator.py):

    return {poft.personID: poft for poft in pofts}

Converting it to something like this should work (untested):

    return dict((poft.personID, poft) for poft in pofts)

Since you wrap all calls to measure_distance in abs, you could just put
the abs in measure_distance.

review: Approve (code)

[Jeroen T. Vermeulen (jtv)]

It's a good thing you spot these things. A few months on 2.7 and I'm forgetting that we still need 2.6.

The hard part of making measure_distance return an absolute value is coming up with a good name. I arrived at the verb “to size”: size_distance. Which also happens to be shorter.

[Jeroen T. Vermeulen (jtv)]

Late-breaking change: after a chat with StevenK, I turned this into a Garbo loop. See revisions 15215 and beyond.

Note that the “launchpad” database user does not have the privilege of deleting POFileTranslator records. I could just add that to facilitate testing, but it's going a bit far. Much as I hate the finicky SELECT restrictions of security.cfg, I do like the restricted write privileges for specialized tables.

[Benji York (benji)]

The changes look good to me. I can get behind fewer methods and more functions.

review: Approve (code)

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2012-05-09 13:50:03 +0000
+++ database/schema/security.cfg	2012-05-24 05:03:19 +0000
@@ -2181,6 +2181,7 @@
 public.openidconsumerassociation        = SELECT, DELETE
 public.openidconsumernonce              = SELECT, DELETE
 public.person                           = SELECT, DELETE
+public.pofiletranslator                 = SELECT, INSERT, UPDATE, DELETE
 public.potranslation                    = SELECT, DELETE
 public.potmsgset                        = SELECT, DELETE
 public.product                          = SELECT

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2012-05-09 01:35:41 +0000
+++ lib/lp/scripts/garbo.py	2012-05-24 05:03:19 +0000
@@ -105,6 +105,9 @@
 from lp.translations.model.translationtemplateitem import (
     TranslationTemplateItem,
     )
+from lp.translations.scripts.scrub_pofiletranslator import (
+    ScrubPOFileTranslator,
+    )
 
 
 ONE_DAY_IN_SECONDS = 24 * 60 * 60
@@ -1418,6 +1421,7 @@
         ObsoleteBugAttachmentPruner,
         OldTimeLimitedTokenDeleter,
         RevisionAuthorEmailLinker,
+        ScrubPOFileTranslator,
         SuggestiveTemplatesCacheUpdater,
         POTranslationPruner,
         UnusedPOTMsgSetPruner,

=== added file 'lib/lp/translations/scripts/scrub_pofiletranslator.py'
--- lib/lp/translations/scripts/scrub_pofiletranslator.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/scripts/scrub_pofiletranslator.py	2012-05-24 05:03:19 +0000
@@ -0,0 +1,161 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Keep `POFileTranslator` more or less consistent with the real data."""
+
+__metaclass__ = type
+__all__ = [
+    'ScrubPOFileTranslator',
+    ]
+
+from storm.expr import (
+    Coalesce,
+    Desc,
+    )
+import transaction
+
+from lp.services.database.lpstorm import IStore
+from lp.services.looptuner import TunableLoop
+from lp.translations.model.pofile import POFile
+from lp.translations.model.pofiletranslator import POFileTranslator
+from lp.translations.model.potemplate import POTemplate
+from lp.translations.model.translationmessage import TranslationMessage
+from lp.translations.model.translationtemplateitem import (
+    TranslationTemplateItem,
+    )
+
+
+def get_pofiles():
+    """Retrieve POFiles to scrub.
+
+    The result's ordering is aimed at maximizing cache effectiveness:
+    by POTemplate name for locality of shared POTMsgSets, and by language
+    for locality of shared TranslationMessages.
+    """
+    store = IStore(POFile)
+    query = store.find(
+        POFile,
+        POFile.potemplateID == POTemplate.id,
+        POTemplate.iscurrent == True)
+    return query.order_by(POTemplate.name, POFile.languageID)
+
+
+def get_contributions(pofile):
+    """Map all users' most recent contributions to `pofile`.
+
+    Returns a dict mapping `Person` id to the creation time of their most
+    recent `TranslationMessage` in `POFile`.
+
+    This leaves some small room for error: a contribution that is masked by
+    a diverged entry in this POFile will nevertheless produce a
+    POFileTranslator record.  Fixing that would complicate the work more than
+    it is probably worth.
+    """
+    store = IStore(pofile)
+    potmsgset_ids = store.find(
+        TranslationTemplateItem.potmsgsetID,
+        TranslationTemplateItem.potemplateID == pofile.potemplate.id,
+        TranslationTemplateItem.sequence > 0)
+    contribs = store.find(
+        (TranslationMessage.submitterID, TranslationMessage.date_created),
+        TranslationMessage.potmsgsetID.is_in(potmsgset_ids),
+        TranslationMessage.languageID == pofile.language.id,
+        TranslationMessage.msgstr0 != None,
+        Coalesce(
+            TranslationMessage.potemplateID,
+            pofile.potemplate.id) == pofile.potemplate.id)
+    contribs = contribs.config(distinct=(TranslationMessage.submitterID,))
+    contribs = contribs.order_by(
+        TranslationMessage.submitterID, Desc(TranslationMessage.date_created))
+    return dict(contribs)
+
+
+def get_pofiletranslators(pofile):
+    """Get `POFileTranslator` entries for `pofile`.
+
+    Returns a dict mapping each contributor's person id to their
+    `POFileTranslator` record.
+    """
+    store = IStore(pofile)
+    pofts = store.find(
+        POFileTranslator, POFileTranslator.pofileID == pofile.id)
+    return dict((poft.personID, poft) for poft in pofts)
+
+
+def remove_pofiletranslators(logger, pofile, person_ids):
+    """Delete `POFileTranslator` records."""
+    logger.debug(
+        "Removing %d POFileTranslator(s) for %s.",
+        len(person_ids), pofile.title)
+    store = IStore(pofile)
+    pofts = store.find(
+        POFileTranslator,
+        POFileTranslator.pofileID == pofile.id,
+        POFileTranslator.personID.is_in(person_ids))
+    pofts.remove()
+
+
+def remove_unwarranted_pofiletranslators(logger, pofile, pofts, contribs):
+    """Delete `POFileTranslator` records that shouldn't be there."""
+    excess = set(pofts) - set(contribs)
+    if len(excess) > 0:
+        remove_pofiletranslators(logger, pofile, excess)
+
+
+def create_missing_pofiletranslators(logger, pofile, pofts, contribs):
+    """Create `POFileTranslator` records that were missing."""
+    shortage = set(contribs) - set(pofts)
+    if len(shortage) == 0:
+        return
+    logger.debug(
+        "Adding %d POFileTranslator(s) for %s.",
+        len(shortage), pofile.title)
+    store = IStore(pofile)
+    for missing_contributor in shortage:
+        store.add(POFileTranslator(
+            pofile=pofile, personID=missing_contributor,
+            date_last_touched=contribs[missing_contributor]))
+
+
+def scrub_pofile(logger, pofile):
+    """Scrub `POFileTranslator` entries for one `POFile`.
+
+    Removes inappropriate entries and adds missing ones.
+    """
+    contribs = get_contributions(pofile)
+    pofiletranslators = get_pofiletranslators(pofile)
+    remove_unwarranted_pofiletranslators(
+        logger, pofile, pofiletranslators, contribs)
+    create_missing_pofiletranslators(
+        logger, pofile, pofiletranslators, contribs)
+
+
+class ScrubPOFileTranslator(TunableLoop):
+    """Tunable loop, meant for running from inside Garbo."""
+
+    maximum_chunk_size = 500
+
+    def __init__(self, *args, **kwargs):
+        super(ScrubPOFileTranslator, self).__init__(*args, **kwargs)
+        # This does not listify the POFiles; they are batch-fetched on
+        # demand.  So iteration may not be entirely exact, but it
+        # doesn't really need to be.  It avoids loading all those
+        # POFiles into memory when we only need a few per iteration.
+        self.pofiles = get_pofiles()
+        self.next_offset = 0
+
+    def __call__(self, chunk_size):
+        """See `ITunableLoop`."""
+        start_offset = self.next_offset
+        self.next_offset = start_offset + int(chunk_size)
+        batch = list(self.pofiles[start_offset:self.next_offset])
+        if len(batch) == 0:
+            self.next_offset = None
+        else:
+            for pofile in batch:
+                scrub_pofile(self.log, pofile)
+            transaction.commit()
+
+    def isDone(self):
+        """See `ITunableLoop`."""
+        return self.next_offset is None

=== added file 'lib/lp/translations/scripts/tests/test_scrub_pofiletranslator.py'
--- lib/lp/translations/scripts/tests/test_scrub_pofiletranslator.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/scripts/tests/test_scrub_pofiletranslator.py	2012-05-24 05:03:19 +0000
@@ -0,0 +1,219 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test scrubbing of `POFileTranslator`."""
+
+__metaclass__ = type
+
+from datetime import (
+    datetime,
+    timedelta,
+    )
+
+import pytz
+import transaction
+
+from lp.services.database.constants import UTC_NOW
+from lp.services.database.lpstorm import IStore
+from lp.services.log.logger import DevNullLogger
+from lp.testing import TestCaseWithFactory
+from lp.testing.layers import ZopelessDatabaseLayer
+from lp.translations.model.pofiletranslator import POFileTranslator
+from lp.translations.scripts.scrub_pofiletranslator import (
+    get_contributions,
+    get_pofiles,
+    get_pofiletranslators,
+    scrub_pofile,
+    ScrubPOFileTranslator,
+    )
+
+
+fake_logger = DevNullLogger()
+
+
+def size_distance(sequence, item1, item2):
+    """Return the absolute distance between items in a sequence."""
+    container = list(sequence)
+    return abs(container.index(item2) - container.index(item1))
+
+
+class TestScrubPOFileTranslator(TestCaseWithFactory):
+
+    layer = ZopelessDatabaseLayer
+
+    def query_pofiletranslator(self, pofile, person):
+        """Query `POFileTranslator` for a specific record.
+
+        :return: Storm result set.
+        """
+        store = IStore(pofile)
+        return store.find(POFileTranslator, pofile=pofile, person=person)
+
+    def make_message_with_pofiletranslator(self, pofile=None):
+        """Create a normal `TranslationMessage` with `POFileTranslator`."""
+        if pofile is None:
+            pofile = self.factory.makePOFile()
+        potmsgset = self.factory.makePOTMsgSet(
+            potemplate=pofile.potemplate, sequence=1)
+        # A database trigger on TranslationMessage automatically creates
+        # a POFileTranslator record for each new TranslationMessage.
+        return self.factory.makeSuggestion(pofile=pofile, potmsgset=potmsgset)
+
+    def make_message_without_pofiletranslator(self, pofile=None):
+        """Create a `TranslationMessage` without `POFileTranslator`."""
+        tm = self.make_message_with_pofiletranslator(pofile)
+        IStore(pofile).flush()
+        self.becomeDbUser('postgres')
+        self.query_pofiletranslator(pofile, tm.submitter).remove()
+        return tm
+
+    def make_pofiletranslator_without_message(self, pofile=None):
+        """Create a `POFileTranslator` without `TranslationMessage`."""
+        if pofile is None:
+            pofile = self.factory.makePOFile()
+        poft = POFileTranslator(
+            pofile=pofile, person=self.factory.makePerson(),
+            date_last_touched=UTC_NOW)
+        IStore(poft.pofile).add(poft)
+        return poft
+
+    def test_get_pofiles_gets_pofiles_for_active_templates(self):
+        pofile = self.factory.makePOFile()
+        self.assertIn(pofile, get_pofiles())
+
+    def test_get_pofiles_skips_inactive_templates(self):
+        pofile = self.factory.makePOFile()
+        pofile.potemplate.iscurrent = False
+        self.assertNotIn(pofile, get_pofiles())
+
+    def test_get_pofiles_clusters_by_template_name(self):
+        # POFiles for templates with the same name are bunched together
+        # in the get_pofiles() output.
+        templates = [
+            self.factory.makePOTemplate(name='shared'),
+            self.factory.makePOTemplate(name='other'),
+            self.factory.makePOTemplate(name='andanother'),
+            self.factory.makePOTemplate(
+                name='shared', distroseries=self.factory.makeDistroSeries()),
+            ]
+        pofiles = [
+            self.factory.makePOFile(potemplate=template)
+            for template in templates]
+        ordering = get_pofiles()
+        self.assertEqual(1, size_distance(ordering, pofiles[0], pofiles[-1]))
+
+    def test_get_pofiles_clusters_by_language(self):
+        # POFiles for sharing templates and the same language are
+        # bunched together in the get_pofiles() output.
+        templates = [
+            self.factory.makePOTemplate(
+                name='shared', distroseries=self.factory.makeDistroSeries())
+            for counter in range(2)]
+        # POFiles per language & template.  We create these in a strange
+        # way to avoid the risk of mistaking accidental orderings such
+        # as per-id from being mistaken for the proper order.
+        languages = ['nl', 'fr']
+        pofiles_per_language = dict((language, []) for language in languages)
+        for language, pofiles in pofiles_per_language.items():
+            for template in templates:
+                pofiles.append(
+                    self.factory.makePOFile(language, potemplate=template))
+
+        ordering = get_pofiles()
+        for pofiles in pofiles_per_language.values():
+            self.assertEqual(
+                1, size_distance(ordering, pofiles[0], pofiles[1]))
+
+    def test_get_contributions_gets_contributions(self):
+        pofile = self.factory.makePOFile()
+        tm = self.factory.makeSuggestion(pofile=pofile)
+        self.assertEqual(
+            {tm.submitter.id: tm.date_created}, get_contributions(pofile))
+
+    def test_get_contributions_uses_latest_contribution(self):
+        pofile = self.factory.makePOFile()
+        today = datetime.now(pytz.UTC)
+        yesterday = today - timedelta(1, 1, 1)
+        old_tm = self.factory.makeSuggestion(
+            pofile=pofile, date_created=yesterday)
+        new_tm = self.factory.makeSuggestion(
+            translator=old_tm.submitter, pofile=pofile, date_created=today)
+        self.assertNotEqual(old_tm.date_created, new_tm.date_created)
+        self.assertContentEqual(
+            [new_tm.date_created], get_contributions(pofile).values())
+
+    def test_get_contributions_ignores_inactive_potmsgsets(self):
+        pofile = self.factory.makePOFile()
+        potmsgset = self.factory.makePOTMsgSet(
+            potemplate=pofile.potemplate, sequence=0)
+        self.factory.makeSuggestion(pofile=pofile, potmsgset=potmsgset)
+        self.assertEqual({}, get_contributions(pofile))
+
+    def test_get_contributions_includes_diverged_messages_for_template(self):
+        pofile = self.factory.makePOFile()
+        tm = self.factory.makeSuggestion(pofile=pofile)
+        tm.potemplate = pofile.potemplate
+        self.assertContentEqual(
+            [tm.submitter.id], get_contributions(pofile).keys())
+
+    def test_get_contributions_excludes_other_diverged_messages(self):
+        pofile = self.factory.makePOFile()
+        tm = self.factory.makeSuggestion(pofile=pofile)
+        tm.potemplate = self.factory.makePOTemplate()
+        self.assertEqual({}, get_contributions(pofile))
+
+    def test_get_pofiletranslators_gets_pofiletranslators_for_pofile(self):
+        pofile = self.factory.makePOFile()
+        tm = self.make_message_with_pofiletranslator(pofile)
+        pofts = get_pofiletranslators(pofile)
+        self.assertContentEqual([tm.submitter.id], pofts.keys())
+        poft = pofts[tm.submitter.id]
+        self.assertEqual(pofile, poft.pofile)
+
+    def test_scrub_pofile_leaves_good_pofiletranslator_in_place(self):
+        pofile = self.factory.makePOFile()
+        tm = self.make_message_with_pofiletranslator(pofile)
+        old_poft = self.query_pofiletranslator(pofile, tm.submitter).one()
+
+        scrub_pofile(fake_logger, pofile)
+
+        new_poft = self.query_pofiletranslator(pofile, tm.submitter).one()
+        self.assertEqual(old_poft, new_poft)
+
+    def test_scrub_pofile_deletes_unwarranted_entries(self):
+        # Deleting POFileTranslator records is not something the app
+        # server ever does, so it requires special privileges.
+        self.becomeDbUser('postgres')
+        poft = self.make_pofiletranslator_without_message()
+        (pofile, person) = (poft.pofile, poft.person)
+        scrub_pofile(fake_logger, poft.pofile)
+        self.assertIsNone(self.query_pofiletranslator(pofile, person).one())
+
+    def test_scrub_pofile_adds_missing_entries(self):
+        pofile = self.factory.makePOFile()
+        tm = self.make_message_without_pofiletranslator(pofile)
+
+        scrub_pofile(fake_logger, pofile)
+
+        new_poft = self.query_pofiletranslator(pofile, tm.submitter).one()
+        self.assertEqual(tm.submitter, new_poft.person)
+        self.assertEqual(pofile, new_poft.pofile)
+
+    def test_tunable_loop(self):
+        pofile = self.factory.makePOFile()
+        tm = self.make_message_without_pofiletranslator(pofile)
+        bad_poft = self.make_pofiletranslator_without_message(pofile)
+        noncontributor = bad_poft.person
+        transaction.commit()
+
+        self.becomeDbUser('garbo')
+        ScrubPOFileTranslator(fake_logger).run()
+        # Try to break the loop if it failed to commit its changes.
+        transaction.abort()
+
+        # The unwarranted POFileTranslator record has been deleted.
+        self.assertIsNotNone(
+            self.query_pofiletranslator(pofile, tm.submitter).one())
+        # The missing POFileTranslator has been created.
+        self.assertIsNone(
+            self.query_pofiletranslator(pofile, noncontributor).one())