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

Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Graham Binns
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~jtv/launchpad/bug-427278
Merge into: lp:launchpad
Diff against target: 246 lines (+57/-15)
5 files modified
lib/lp/code/model/directbranchcommit.py (+17/-3)
lib/lp/code/tests/test_directbranchcommit.py (+1/-1)
lib/lp/translations/doc/translations-export-to-branch.txt (+11/-3)
lib/lp/translations/scripts/translations_to_branch.py (+13/-6)
lib/lp/translations/utilities/gettext_po_exporter.py (+15/-2)
To merge this branch: bzr merge lp:~jtv/launchpad/bug-427278
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Review via email: mp+15147@code.launchpad.net

Commit message

Avoid long idle-in-transaction periods in translations-to-branch export.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

= Bug 427278 =

Second attempt; previously accidentally proposed for merging into db-devel (the default) which polluted the diff with whatever skew there happened to be between devel and db-devel.

Translations exports to bzr branches can sometimes keep transactions open for a while. There's no particular reason to go that far, and it's Not Nice. We've had the script idle-killed a few times.

We've done several things that prevented a repetition:

1. We no longer export all translations for a template; now we only do ones that have changed since the last export. Makes the exports a lot faster.

2. The deadline for long transactions was extended, and obviously that's pretty effective at stopping the alarm bells from ringing. But ideal it is not.

3. The branch you see here makes the script commit more often around the likely pain points.

There's really only one catch: DirectBranchCommit checks the branch for conflicting changes. For that it needs to compare the latest revision in the branch to the last_scanned_id as stored in the database when the DirectBranchCommit was constructed. So instead of getting the last_scanned_id at the point where it's referenced, DirectBranchCommit will now retrieve and store it at construction time. This should also close off a race condition that may or may not exist in the case where database transactions are committed while a DirectBranchCommit is active.

Passing a transaction into DirectBranchCommit.commit, as I do here, is a bit weird. There are reasons for it:

 * Operations that access branch storage can take a long time, but don't need database access. That makes them excellent points to close off any ongoing transaction. The framework starts new transactions on demand, and that's going to happen only after the bzr operation completes. Until that time, the committer avoids being "idle in transaction" on the database.

 * Nowadays txn is global, so we could just "import transaction" and "transaction.commit()". But passing it as an object makes it easy to mock transactions in tests. Also, passing a boolean is slightly more error-prone. Named parameters are a big improvement--"dbc.commit(message, commit=True)"--but still relatively sensitive to mistakes.

 * Passing the transaction object to commit rather than the constructor makes it clear that you're not giving out a blanket license to commit. The DirectBranchCommit can commit only inside that one particular method call. Don't hide transaction boundaries in the program flow.

A lot of the actual diff went into nicer logging.

== Test, demo & Q/A ==

Test:
{{{
./bin/test -vv -t 'translations.*to.*branch' -t 'direct.*branch.*commit'
}}}

Create a branch on staging, and set it as the productseries branch and the translations export branch for some project. Enable translations and two-way bzr synchronization. Push a template to the branch.

Watch the project's import queue; wait for the template to be imported. Then translate a string in the template. Watch the revision log for the branch; an automatic translations commit should appear there within the hour.

No lint.

Jeroen

Revision history for this message
Graham Binns (gmb) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/code/model/directbranchcommit.py'
--- lib/lp/code/model/directbranchcommit.py 2009-09-09 20:03:19 +0000
+++ lib/lp/code/model/directbranchcommit.py 2009-11-26 21:25:26 +0000
@@ -74,6 +74,8 @@
74 self.db_branch = db_branch74 self.db_branch = db_branch
75 self.to_mirror = to_mirror75 self.to_mirror = to_mirror
7676
77 self.last_scanned_id = self.db_branch.last_scanned_id
78
77 if committer is None:79 if committer is None:
78 committer = db_branch.owner80 committer = db_branch.owner
79 self.committer = committer81 self.committer = committer
@@ -172,12 +174,18 @@
172 assert self.is_locked, "Getting revision on un-locked branch."174 assert self.is_locked, "Getting revision on un-locked branch."
173 last_revision = None175 last_revision = None
174 last_revision = self.bzrbranch.last_revision()176 last_revision = self.bzrbranch.last_revision()
175 if last_revision != self.db_branch.last_scanned_id:177 if last_revision != self.last_scanned_id:
176 raise ConcurrentUpdateError(178 raise ConcurrentUpdateError(
177 "Branch has been changed. Not committing.")179 "Branch has been changed. Not committing.")
178180
179 def commit(self, commit_message):181 def commit(self, commit_message, txn=None):
180 """Commit to branch."""182 """Commit to branch.
183
184 :param commit_message: Message for branch's commit log.
185 :param txn: Transaction to commit. Can be helpful in avoiding
186 long idle times in database transactions. May be committed
187 more than once.
188 """
181 assert self.is_open, "Committing closed DirectBranchCommit."189 assert self.is_open, "Committing closed DirectBranchCommit."
182 assert self.is_locked, "Not locked at commit time."190 assert self.is_locked, "Not locked at commit time."
183191
@@ -185,6 +193,9 @@
185 try:193 try:
186 self._checkForRace()194 self._checkForRace()
187195
196 if txn:
197 txn.commit()
198
188 rev_id = self.revision_tree.get_revision_id()199 rev_id = self.revision_tree.get_revision_id()
189 if rev_id == NULL_REVISION:200 if rev_id == NULL_REVISION:
190 if list(self.transform_preview.iter_changes()) == []:201 if list(self.transform_preview.iter_changes()) == []:
@@ -193,6 +204,9 @@
193 self.bzrbranch, commit_message)204 self.bzrbranch, commit_message)
194 IMasterObject(self.db_branch).requestMirror()205 IMasterObject(self.db_branch).requestMirror()
195206
207 if txn:
208 txn.commit()
209
196 finally:210 finally:
197 self.unlock()211 self.unlock()
198 self.is_open = False212 self.is_open = False
199213
=== modified file 'lib/lp/code/tests/test_directbranchcommit.py'
--- lib/lp/code/tests/test_directbranchcommit.py 2009-10-02 09:48:30 +0000
+++ lib/lp/code/tests/test_directbranchcommit.py 2009-11-26 21:25:26 +0000
@@ -37,7 +37,7 @@
3737
38 self.committer = DirectBranchCommit(self.db_branch)38 self.committer = DirectBranchCommit(self.db_branch)
39 if update_last_scanned_id:39 if update_last_scanned_id:
40 self.db_branch.last_scanned_id = (40 self.committer.last_scanned_id = (
41 self.committer.bzrbranch.last_revision())41 self.committer.bzrbranch.last_revision())
4242
43 def _tearDownCommitter(self):43 def _tearDownCommitter(self):
4444
=== modified file 'lib/lp/translations/doc/translations-export-to-branch.txt'
--- lib/lp/translations/doc/translations-export-to-branch.txt 2009-10-20 06:03:46 +0000
+++ lib/lp/translations/doc/translations-export-to-branch.txt 2009-11-26 21:25:26 +0000
@@ -45,7 +45,7 @@
45 ... if self.simulate_race:45 ... if self.simulate_race:
46 ... raise ConcurrentUpdateError("Simulated race condition.")46 ... raise ConcurrentUpdateError("Simulated race condition.")
47 ...47 ...
48 ... def commit(self, message=None):48 ... def commit(self, message=None, txn=None):
49 ... self._checkForRace()49 ... self._checkForRace()
50 ... self.logger.info("Committed %d file(s)." % self.written_files)50 ... self.logger.info("Committed %d file(s)." % self.written_files)
51 ...51 ...
@@ -129,11 +129,11 @@
129129
130 >>> transaction.commit()130 >>> transaction.commit()
131 >>> script = MockExportTranslationsToBranch(131 >>> script = MockExportTranslationsToBranch(
132 ... 'export-to-branch', test_args=['-vv'])132 ... 'export-to-branch', test_args=[])
133 >>> script.main()133 >>> script.main()
134 INFO Exporting to translations branches.134 INFO Exporting to translations branches.
135 INFO Exporting Gazblachko trunk series.135 INFO Exporting Gazblachko trunk series.
136 DEBUG No previous translations commit found.136 DEBUG ...
137 INFO Writing file 'po/main/nl.po':137 INFO Writing file 'po/main/nl.po':
138 INFO # ...138 INFO # ...
139 msgid ""139 msgid ""
@@ -143,6 +143,7 @@
143 msgid "maingazpot msgid"143 msgid "maingazpot msgid"
144 msgstr "maingazpot msgstr"144 msgstr "maingazpot msgstr"
145 <BLANKLINE>145 <BLANKLINE>
146 DEBUG ...
146 INFO Writing file 'po/module/nl.po':147 INFO Writing file 'po/module/nl.po':
147 INFO # ...148 INFO # ...
148 msgid ""149 msgid ""
@@ -152,6 +153,8 @@
152 <BLANKLINE>153 <BLANKLINE>
153 msgid "gazmod msgid"154 msgid "gazmod msgid"
154 msgstr "gazmod msgstr"155 msgstr "gazmod msgstr"
156 <BLANKLINE>
157 DEBUG ...
155 INFO Committed 2 file(s).158 INFO Committed 2 file(s).
156 INFO Unlock.159 INFO Unlock.
157 INFO Processed 1 item(s); 0 failure(s).160 INFO Processed 1 item(s); 0 failure(s).
@@ -186,6 +189,7 @@
186 >>> script.main()189 >>> script.main()
187 INFO Exporting to translations branches.190 INFO Exporting to translations branches.
188 INFO Exporting Gazblachko trunk series.191 INFO Exporting Gazblachko trunk series.
192 DEBUG ....
189 DEBUG Last commit was at ....193 DEBUG Last commit was at ....
190 INFO Unlock.194 INFO Unlock.
191 INFO Processed 1 item(s); 0 failure(s).195 INFO Processed 1 item(s); 0 failure(s).
@@ -197,6 +201,7 @@
197 >>> script.main()201 >>> script.main()
198 INFO Exporting to translations branches.202 INFO Exporting to translations branches.
199 INFO Exporting Gazblachko trunk series.203 INFO Exporting Gazblachko trunk series.
204 DEBUG ....
200 DEBUG Last commit was at ...205 DEBUG Last commit was at ...
201 INFO Writing file 'po/main/nl.po':206 INFO Writing file 'po/main/nl.po':
202 INFO ...207 INFO ...
@@ -228,11 +233,14 @@
228 >>> script.main()233 >>> script.main()
229 INFO Exporting to translations branches.234 INFO Exporting to translations branches.
230 INFO Exporting Gazblachko trunk series.235 INFO Exporting Gazblachko trunk series.
236 DEBUG ....
231 DEBUG No previous translations commit found.237 DEBUG No previous translations commit found.
238 DEBUG ....
232 INFO Writing file 'po/main/nl.po':239 INFO Writing file 'po/main/nl.po':
233 ...240 ...
234 msgstr "gazmod msgstr"241 msgstr "gazmod msgstr"
235 <BLANKLINE>242 <BLANKLINE>
243 DEBUG ...
236 INFO Unlock.244 INFO Unlock.
237 ERROR Failure: Simulated race condition.245 ERROR Failure: Simulated race condition.
238 INFO Processed 1 item(s); 1 failure(s).246 INFO Processed 1 item(s); 1 failure(s).
239247
=== modified file 'lib/lp/translations/scripts/translations_to_branch.py'
--- lib/lp/translations/scripts/translations_to_branch.py 2009-10-20 06:03:46 +0000
+++ lib/lp/translations/scripts/translations_to_branch.py 2009-11-26 21:25:26 +0000
@@ -119,7 +119,7 @@
119 def _commit(self, source, committer):119 def _commit(self, source, committer):
120 """Commit changes to branch. Check for race conditions."""120 """Commit changes to branch. Check for race conditions."""
121 self._checkForObjections(source)121 self._checkForObjections(source)
122 committer.commit(self.commit_message)122 committer.commit(self.commit_message, txn=self.txn)
123123
124 def _isTranslationsCommit(self, revision):124 def _isTranslationsCommit(self, revision):
125 """Is `revision` an automatic translations commit?"""125 """Is `revision` an automatic translations commit?"""
@@ -159,6 +159,9 @@
159 self._checkForObjections(source)159 self._checkForObjections(source)
160160
161 committer = self._prepareBranchCommit(source.translations_branch)161 committer = self._prepareBranchCommit(source.translations_branch)
162 self.logger.debug("Created DirectBranchCommit.")
163 if self.txn:
164 self.txn.commit()
162165
163 bzr_branch = committer.bzrbranch166 bzr_branch = committer.bzrbranch
164167
@@ -185,15 +188,17 @@
185 base_path = os.path.dirname(template.path)188 base_path = os.path.dirname(template.path)
186189
187 for pofile in template.pofiles:190 for pofile in template.pofiles:
188
189 has_changed = (191 has_changed = (
190 changed_since is None or192 changed_since is None or
191 pofile.date_changed > changed_since)193 pofile.date_changed > changed_since)
192 if not has_changed:194 if not has_changed:
193 continue195 continue
194196
197 language_code = pofile.getFullLanguageCode()
198 self.logger.debug("Exporting %s." % language_code)
199
195 pofile_path = os.path.join(200 pofile_path = os.path.join(
196 base_path, pofile.getFullLanguageCode() + '.po')201 base_path, language_code + '.po')
197 pofile_contents = pofile.export()202 pofile_contents = pofile.export()
198203
199 committer.writeFile(pofile_path, pofile_contents)204 committer.writeFile(pofile_path, pofile_contents)
@@ -210,6 +215,7 @@
210 template.clearPOFileCache()215 template.clearPOFileCache()
211216
212 if change_count > 0:217 if change_count > 0:
218 self.logger.debug("Writing to branch.")
213 self._commit(source, committer)219 self._commit(source, committer)
214 finally:220 finally:
215 committer.unlock()221 committer.unlock()
@@ -231,13 +237,14 @@
231 raise237 raise
232 except Exception, e:238 except Exception, e:
233 items_failed += 1239 items_failed += 1
234 self.logger.error("Failure: %s" % e)240 message = unicode(e)
241 if message == u'':
242 message = e.__class__.__name__
243 self.logger.error("Failure: %s" % message)
235 if self.txn:244 if self.txn:
236 self.txn.abort()245 self.txn.abort()
237246
238 items_done += 1247 items_done += 1
239 if self.txn:
240 self.txn.begin()
241248
242 self.logger.info("Processed %d item(s); %d failure(s)." % (249 self.logger.info("Processed %d item(s); %d failure(s)." % (
243 items_done, items_failed))250 items_done, items_failed))
244251
=== modified file 'lib/lp/translations/utilities/gettext_po_exporter.py'
--- lib/lp/translations/utilities/gettext_po_exporter.py 2009-11-02 00:10:05 +0000
+++ lib/lp/translations/utilities/gettext_po_exporter.py 2009-11-26 21:25:26 +0000
@@ -14,6 +14,7 @@
14 'GettextPOExporter',14 'GettextPOExporter',
15 ]15 ]
1616
17import logging
17import os18import os
1819
19from zope.interface import implements20from zope.interface import implements
@@ -344,8 +345,20 @@
344 raise UnicodeEncodeError(345 raise UnicodeEncodeError(
345 '%s:\n%s' % (file_path, str(error)))346 '%s:\n%s' % (file_path, str(error)))
346347
347 # This message cannot be represented in current encoding,348 # This message cannot be represented in the current
348 # change to UTF-8 and try again.349 # encoding.
350 if translation_file.path:
351 file_description = translation_file.path
352 elif translation_file.language_code:
353 file_description = (
354 "%s translation" % translation_file.language_code)
355 else:
356 file_description = "template"
357 logging.info(
358 "Can't represent %s as %s; using UTF-8 instead." % (
359 file_description,
360 translation_file.header.charset.upper()))
361
349 old_charset = translation_file.header.charset362 old_charset = translation_file.header.charset
350 translation_file.header.charset = 'UTF-8'363 translation_file.header.charset = 'UTF-8'
351 # We need to update the header too.364 # We need to update the header too.