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:
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
