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

[Jeroen T. Vermeulen]

[r=jcsackett,lifeless][ui=none][bug=702228] Avoid long-running master transactions during translations export.

= Bug 702228 =

A particularly difficult translations export request was breaking the export queue. It took over 3 hours to run, at which point our watchdog script that looks for long transactions on the master database killed its master store connection.

This branch addresses that by avoiding use of the master database, so that the connection will not start a transaction until the very end of a request (where a master connection is really needed). An occasional long transaction on the slave is not such a big problem, and certainly won't take any write locks. So the watchdog will leave the slave connection in peace.

There is a nesting of database policies: the SlaveDatabasePolicy makes the slave store the default, so that e.g. some use of cursor() that we still have will use the slave. This is merely a little help in reducing accidental use of the master store. But I also bracketed the time-critical part in a SlaveOnlyDatabasePolicy. This outright disallows use of the master store.

It's not quite obvious, but the method that fetches the next request off the queue accesses the master store. It doesn't make any changes there, but it does verify that the oldest fully-replicated request it gets off the slave store has not already been deleted from the master store. That situation could otherwise occur if the script deleted the previous request from the master store, then again tried to fetch the oldest request off the slave before the deletion had replicated.

I tested thusly:
{{{
./bin/test -vvc lp.translations -t export
}}}

To Q/A, first ensure that regular exports still work (both in Ubuntu and upstream; include XPI) by running the script manually, and seeing that it still produces valid tarballs and individual files. On staging that requires manually clearing the backlog first.

Watch pg_stat_activity on the master database during a large export. The export script's master backend process (as logged by the script) should be "<IDLE>" until the end, not "<IDLE> in transaction."

No lint,

Jeroen

[Robert Collins (lifeless)]

review: Approve

[j.c.sackett (jcsackett)]

Jeroen--

This looks good. I have couple comments below, but the one on the test case is the only one I feel is important. This is good to land with that change.

> === modified file 'lib/lp/translations/scripts/po_export_queue.py'
> --- lib/lp/translations/scripts/po_export_queue.py 2010-08-20 20:31:18 +0000
> +++ lib/lp/translations/scripts/po_export_queue.py 2011-01-13 18:25:08 +0000
> @@ -250,6 +252,46 @@
> 'request_url': self.request_url,
> }
>
> + def setExportFile(self, exported_file):
> + """Attach an exported file to the result, for upload to the Librarian.
> +
> + After this is set, `upload` will perform the actual upload. The two
> + actions are separated so as to isolate write access to the database.
> +
> + :param exported_file: An `IExportedTranslationFile` containing the
> + exported data.
> + """
> + self.exported_file = exported_file

Given how very little this method is doing, I'm not sure it needs to be a separate method. It's not hurting anything to have it this way, but seems slightly unnecesary.

> === added file 'lib/lp/translations/tests/test_exportresult.py'
> --- lib/lp/translations/tests/test_exportresult.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/translations/tests/test_exportresult.py 2011-01-13 18:25:08 +0000
> @@ -0,0 +1,67 @@
> +# Copyright 2010 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Tests for `ExportResult`."""
> +
> +__metaclass__ = type
> +
> +import hashlib
> +import os.path
> +from StringIO import StringIO
> +
> +from canonical.librarian.testing.fake import FakeLibrarian
> +from canonical.testing.layers import ZopelessDatabaseLayer
> +from lp.services.log.logger import DevNullLogger
> +from lp.testing import TestCaseWithFactory
> +from lp.translations.scripts.po_export_queue import ExportResult
> +
> +
> +class FakeExportedTranslationFile:
> + """Fake `IExportedTranslationFile` for testing."""
> +
> + def __init__(self, path, content, content_type):
> + self.path = path
> + base, ext = os.path.splitext(path)
> + self.file_extension = ext
> + self.size = len(content)
> + self.content = content
> + self.file = StringIO(content)
> + self.content_type = content_type
> +
> + def read(self, *args, **kwargs):
> + return self.file.read(*args, **kwargs)
> +
> +
> +class TestExportResult(TestCaseWithFactory):
> +
> + layer = ZopelessDatabaseLayer
> +
> + def makeExportResult(self):
> + request = [self.factory.makePOFile()]
> + requester = self.factory.makePerson()
> + logger = DevNullLogger()
> + return ExportResult(requester, request, logger)
> +
> + def makeExportedTranslationFile(self):
> + filename = self.factory.getUniqueString()
> + content = self.factory.getUniqueString()
> + mime_type = "text/plain"
> + return FakeExportedTranslationFile(filename, content, mime_type)
> +
> + def test_upload_exported_file(self):
> + librarian = self.useFixture(FakeLibrarian())
> + export = self.makeExp...

review: Approve (code*)

[Jeroen T. Vermeulen (jtv)]

You're right about the assertNotEqual. Fixed with thanks.

You may also be right about the other method, but I can't think of anything to do about it. I wouldn't want to set the property on the object from outside the object's own code: I feel that would be highly nonobvious, sort of like an out-of-place parameter. So I left that as it was.

Jeroen

=== modified file 'cronscripts/rosetta-export-queue.py'
--- cronscripts/rosetta-export-queue.py	2010-04-27 19:48:39 +0000
+++ cronscripts/rosetta-export-queue.py	2011-01-13 23:31:34 +0000
@@ -8,17 +8,20 @@
 import _pythonpath
 
 from canonical.database.sqlbase import ISOLATION_LEVEL_READ_COMMITTED
+from canonical.launchpad.webapp.dbpolicy import SlaveDatabasePolicy
 from lp.translations.scripts.po_export_queue import process_queue
 from lp.services.scripts.base import LaunchpadCronScript
 
 
 class RosettaExportQueue(LaunchpadCronScript):
+    """Translation exports."""
+
     def main(self):
         self.txn.set_isolation_level(ISOLATION_LEVEL_READ_COMMITTED)
-        process_queue(self.txn, self.logger)
+        with SlaveDatabasePolicy():
+            process_queue(self.txn, self.logger)
 
 
 if __name__ == '__main__':
     script = RosettaExportQueue('rosetta-export-queue', dbuser='poexport')
     script.lock_and_run()
-
 
=== modified file 'lib/lp/translations/doc/poexport-queue.txt'
--- lib/lp/translations/doc/poexport-queue.txt	2010-12-02 16:13:51 +0000
+++ lib/lp/translations/doc/poexport-queue.txt	2011-01-13 23:31:34 +0000
@@ -42,7 +42,7 @@
     ...     result.addFailure()
 
 In this case, there is an error, so there shouldn't be a URL to download
-anything, if we set it, the system will fail:
+anything.  If we set it, the system will fail:
 
     >>> result.url = 'http://someplace.com/somefile.tar.gz'
 
 
=== modified file 'lib/lp/translations/scripts/po_export_queue.py'
--- lib/lp/translations/scripts/po_export_queue.py	2010-08-20 20:31:18 +0000
+++ lib/lp/translations/scripts/po_export_queue.py	2011-01-13 23:31:34 +0000
@@ -23,6 +23,7 @@
 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
 from canonical.launchpad.mail import simple_sendmail
 from canonical.launchpad.webapp import canonical_url
+from canonical.launchpad.webapp.dbpolicy import SlaveOnlyDatabasePolicy
 from lp.registry.interfaces.productseries import IProductSeries
 from lp.registry.interfaces.sourcepackage import ISourcePackage
 from lp.translations.interfaces.poexportrequest import IPOExportRequestSet
@@ -54,6 +55,7 @@
         self.url = None
         self.failure = None
         self.logger = logger
+        self.exported_file = None
 
         self.requested_exports = list(requested_exports)
         export_requested_at = self._getExportRequestOrigin()
@@ -250,6 +252,46 @@
             'request_url': self.request_url,
             }
 
+    def setExportFile(self, exported_file):
+        """Attach an exported file to the result, for upload to the Librarian.
+
+        After this is set, `upload` will perform the actual upload.  The two
+        actions are separated so as to isolate write access to the database.
+
+        :param exported_file: An `IExportedTranslationFile` containing the
+            exported data.
+        """
+        self.exported_file = exported_file
+
+    def upload(self, logger=None):
+        """Upload exported file as set with `setExportFile` to the Librarian.
+
+        If no file has been set, do nothing.
+        """
+        if self.exported_file is None:
+            # There's nothing to upload.
+            return
+
+        if self.exported_file.path is None:
+            # The exported path is unknown, use translation domain as its
+            # filename.
+            assert self.exported_file.file_extension, (
+                'File extension must have a value!.')
+            path = 'launchpad-export.%s' % self.exported_file.file_extension
+        else:
+            # Convert the path to a single file name so it's noted in
+            # librarian.
+            path = self.exported_file.path.replace(os.sep, '_')
+
+        alias_set = getUtility(ILibraryFileAliasSet)
+        alias = alias_set.create(
+            name=path, size=self.exported_file.size, file=self.exported_file,
+            contentType=self.exported_file.content_type)
+
+        self.url = alias.http_url
+        if logger is not None:
+            logger.info("Stored file at %s" % self.url)
+
     def notify(self):
         """Send a notification email to the given person about the export.
 
@@ -331,6 +373,9 @@
     multiple files) and information about files that we failed to export (if
     any).
     """
+    # Keep as much work off the master store as possible, so we avoid
+    # opening unnecessary transactions there.  It could be a while
+    # before we get to the commit.
     translation_exporter = getUtility(ITranslationExporter)
     requested_objects = list(objects)
 
@@ -352,28 +397,9 @@
         # error, we add the entry to the list of errors.
         result.addFailure()
     else:
-        if exported_file.path is None:
-            # The exported path is unknown, use translation domain as its
-            # filename.
-            assert exported_file.file_extension, (
-                'File extension must have a value!.')
-            exported_path = 'launchpad-export.%s' % (
-                exported_file.file_extension)
-        else:
-            # Convert the path to a single file name so it's noted in
-            # librarian.
-            exported_path = exported_file.path.replace(os.sep, '_')
-
-        alias_set = getUtility(ILibraryFileAliasSet)
-        alias = alias_set.create(
-            name=exported_path,
-            size=exported_file.size,
-            file=exported_file,
-            contentType=exported_file.content_type)
-        result.url = alias.http_url
-        logger.info("Stored file at %s" % result.url)
-
-    result.notify()
+        result.setExportFile(exported_file)
+
+    return result
 
 
 def process_queue(transaction_manager, logger):
@@ -386,8 +412,19 @@
 
     request = request_set.getRequest()
     while request != no_request:
-        person, objects, format, request_ids = request
-        process_request(person, objects, format, logger)
+
+        # This can take a long time.  Make sure we don't open any
+        # transactions on the master store before we really need to.
+        transaction_manager.commit()
+        with SlaveOnlyDatabasePolicy():
+            person, objects, format, request_ids = request
+            result = process_request(person, objects, format, logger)
+
+        # Almost done.  Now we can go back to using the master database
+        # where needed.
+        result.upload(logger=logger)
+        result.notify()
+
         request_set.removeRequest(request_ids)
         transaction_manager.commit()
 
 
=== added file 'lib/lp/translations/tests/test_exportresult.py'
--- lib/lp/translations/tests/test_exportresult.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/tests/test_exportresult.py	2011-01-13 23:31:34 +0000
@@ -0,0 +1,67 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `ExportResult`."""
+
+__metaclass__ = type
+
+import hashlib
+import os.path
+from StringIO import StringIO
+
+from canonical.librarian.testing.fake import FakeLibrarian
+from canonical.testing.layers import ZopelessDatabaseLayer
+from lp.services.log.logger import DevNullLogger
+from lp.testing import TestCaseWithFactory
+from lp.translations.scripts.po_export_queue import ExportResult
+
+
+class FakeExportedTranslationFile:
+    """Fake `IExportedTranslationFile` for testing."""
+
+    def __init__(self, path, content, content_type):
+        self.path = path
+        base, ext = os.path.splitext(path)
+        self.file_extension = ext
+        self.size = len(content)
+        self.content = content
+        self.file = StringIO(content)
+        self.content_type = content_type
+
+    def read(self, *args, **kwargs):
+        return self.file.read(*args, **kwargs)
+
+
+class TestExportResult(TestCaseWithFactory):
+
+    layer = ZopelessDatabaseLayer
+
+    def makeExportResult(self):
+        request = [self.factory.makePOFile()]
+        requester = self.factory.makePerson()
+        logger = DevNullLogger()
+        return ExportResult(requester, request, logger)
+
+    def makeExportedTranslationFile(self):
+        filename = self.factory.getUniqueString()
+        content = self.factory.getUniqueString()
+        mime_type = "text/plain"
+        return FakeExportedTranslationFile(filename, content, mime_type)
+
+    def test_upload_exported_file(self):
+        librarian = self.useFixture(FakeLibrarian())
+        export = self.makeExportedTranslationFile()
+        export_result = self.makeExportResult()
+        export_result.setExportFile(export)
+        export_result.upload()
+
+        self.assertIsNot(None, export_result.url)
+        sha1 = hashlib.sha1(export.content).hexdigest()
+        self.assertEqual(sha1, librarian.aliases.values()[0].content.sha1)
+        alias = librarian.findBySHA1(sha1)
+        self.assertEqual(export.path, alias.filename)
+
+    def test_upload_without_exported_file_does_nothing(self):
+        export_result = self.makeExportResult()
+        export_result.upload()
+        self.assertIs(None, export_result.url)