launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-812500 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #812500 in Launchpad itself: "translations to branch script failing with InternalError: transaction is read-only "
  https://bugs.launchpad.net/launchpad/+bug/812500

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-812500/+merge/79373

= Summary =

The translations-to-branch exporter has been quietly spitting out errors because on some branches, it tried to write to the slave database.

This happens when the script discovers that a branch contains changes that haven't been scanned yet.  The scan may still be pending or still in progress, but it's also possible that the previous scan got interrupted somehow and needs to be re-done.  The re-scan request is kept as a property of the Branch object in the database, but the script normally operates on a slave-store copy for performance/scalability reasons.  You see the problem.


== Proposed fix ==

When requesting a re-scan, do so on a master-store copy of the branch.


== Pre-implementation notes ==

When a branch needs re-scanning, one explanation may be that the system is under high load.  I was worried that requesting a re-scan might accumulate more load, but Robert tells me that is not a concern.  The added load is negligible.

In the longer run, it seems wrong to have to deal with broken branches in a script that merely happens to discover (some of) them.  This deserves active data scrubbing; all the translations-to-branch export script should have to do when it finds a broken branch is move on and wait for someone else to fix anything that's durably broken.


== Implementation details ==

You may be worried that the re-scan request is based on information from the slave store, which may be lagged.  But as far as I can tell, the information that goes into the request is based purely on the bzr branch in the code repository, not the Branch object in the database.


== Tests ==

{{{
./bin/test -vvc lp.translations.scripts.tests.test_translations_to_branch -t exportToStaleBranch
}}}


== Demo and Q/A ==

Push a branch to a codehosting server (but do not run the branch scanner) to ensure that there's a stale branch.  Then run cronscripts/translations-export-to-branch.py.  If this is a test server, it probably won't have many actual bzr branches so it will skip almost everything at a very high pace.  But for your branch, it should note that the branch is stale and skip it — without that "read-only database" error.


= Launchpad lint =

There's one warning (as in many other places in LP) about an import of lp.codehosting that's not used as an import, but only for its side effects.  Not nice, but definitely out of scope.


Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/scripts/tests/test_translations_to_branch.py
  lib/lp/translations/scripts/translations_to_branch.py

./lib/lp/translations/scripts/translations_to_branch.py
      48: 'lp' imported but unused
-- 
https://code.launchpad.net/~jtv/launchpad/bug-812500/+merge/79373
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-812500 into lp:launchpad.

=== modified file 'lib/lp/translations/scripts/tests/test_translations_to_branch.py'
--- lib/lp/translations/scripts/tests/test_translations_to_branch.py	2011-05-26 15:44:09 +0000
+++ lib/lp/translations/scripts/tests/test_translations_to_branch.py	2011-10-14 08:10:05 +0000
@@ -1,20 +1,21 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Acceptance test for the translations-export-to-branch script."""
 
 import datetime
-import pytz
 import re
 from textwrap import dedent
 
 from bzrlib.errors import NotBranchError
+import pytz
 from testtools.matchers import MatchesRegex
 import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
+from canonical.launchpad.interfaces.lpstorm import ISlaveStore
 from canonical.launchpad.scripts.tests import run_script
 from canonical.testing.layers import ZopelessAppServerLayer
 from lp.app.enums import ServiceUsage
@@ -22,6 +23,7 @@
     ITeamMembershipSet,
     TeamMembershipStatus,
     )
+from lp.registry.model.productseries import ProductSeries
 from lp.services.log.logger import BufferLogger
 from lp.testing import (
     map_branch_contents,
@@ -158,7 +160,11 @@
         self.assertFalse(db_branch.pending_writes)
         self.assertNotEqual(
             db_branch.last_mirrored_id, tree.branch.last_revision())
-        exporter._exportToBranch(productseries)
+        # The export code works on a Branch from the slave store.  It
+        # shouldn't stop the scan request.
+        slave_series = ISlaveStore(productseries).get(
+            ProductSeries, productseries.id)
+        exporter._exportToBranch(slave_series)
         self.assertEqual(
             db_branch.last_mirrored_id, tree.branch.last_revision())
         self.assertTrue(db_branch.pending_writes)

=== modified file 'lib/lp/translations/scripts/translations_to_branch.py'
--- lib/lp/translations/scripts/translations_to_branch.py	2011-10-10 16:53:10 +0000
+++ lib/lp/translations/scripts/translations_to_branch.py	2011-10-14 08:10:05 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Export translation snapshots to bzr branches where requested."""
@@ -28,6 +28,7 @@
     get_email_template,
     shortlist,
     )
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.launchpad.webapp import errorlog
 from canonical.launchpad.webapp.interfaces import (
     IStoreSelector,
@@ -35,15 +36,16 @@
     SLAVE_FLAVOR,
     )
 from lp.app.enums import ServiceUsage
-# Load the normal plugin set. Lint complains but keep this in.
-import lp.codehosting
 from lp.code.errors import StaleLastMirrored
 from lp.code.interfaces.branch import get_db_branch_info
 from lp.code.interfaces.branchjob import IRosettaUploadJobSource
+from lp.code.model.branch import Branch
 from lp.code.model.directbranchcommit import (
     ConcurrentUpdateError,
     DirectBranchCommit,
     )
+# Load the normal plugin set. Lint complains but keep this in.
+import lp.codehosting
 from lp.codehosting.vfs import get_rw_server
 from lp.services.mail.sendmail import (
     format_address,
@@ -195,14 +197,20 @@
         self.logger.info("Exporting %s." % source.title)
         self._checkForObjections(source)
 
+        branch = source.translations_branch
+
         try:
-            committer = self._prepareBranchCommit(source.translations_branch)
+            committer = self._prepareBranchCommit(branch)
         except StaleLastMirrored as e:
-            source.translations_branch.branchChanged(
-                **get_db_branch_info(**e.info))
+            # Request a rescan of the branch.  Do this on the master
+            # store, or we won't be able to modify the branch object.
+            # (The master copy may also be more recent, in which case
+            # the rescan won't be necessary).
+            master_branch = IMasterStore(branch).get(Branch, branch.id)
+            master_branch.branchChanged(**get_db_branch_info(**e.info))
             self.logger.warning(
                 'Skipped %s due to stale DB info and scheduled scan.',
-                source.translations_branch.bzr_identity)
+                branch.bzr_identity)
             if self.txn:
                 self.txn.commit()
             return
@@ -281,7 +289,7 @@
                 self._handleUnpushedBranch(source)
                 if self.txn:
                     self.txn.commit()
-            except Exception, e:
+            except Exception as e:
                 items_failed += 1
                 self.logger.error("Failure: %s" % repr(e))
                 if self.txn: