launchpad-reviewers team mailing list archive

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

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #719267 TranslationImportQueueEntry:+index timeouts approving translation templates
  https://bugs.launchpad.net/bugs/719267

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

= Summary =

Timeouts were blocking translations import queue reviews, at least the creation of new templates.  The time went into recalculating the statistics of each POFile that is created to match ones in sharing templates.  Some large queries are involved.


== Proposed fix ==

A new template is created empty, so there's no need to query the database — all the counts that get constructed directly from the queries are zero.

I considered adding a parameter for "don't bother calculating the statistics," since the problem happens in a known scenario where the statistics don't need calculating.  But in the end I just created some shortcuts: if the template is empty, the queries are skipped and zero counts are returned.  This doesn't affect any APIs and may even benefit additional invocations.


== Pre-implementation notes ==

Robert looked into optimizing statistics calculation for multiple languages at once.  The results are promising, though in this case there's no need to go to the trouble.


== Implementation details ==

The call that led to the unnecessary queries had an unnecessary line break.  There is no other reason why I changed it.


== Tests ==

The statistics as calculated by the changed methods are extensively tested by:
{{{
./bin/test -vvc lp.translations.tests.test_pofile
}}}

I'm adding one for the zero case, just in case it's not properly covered: test_updateStatistics_counts_zero_for_empty_template.


== Demo and Q/A ==

Approve the oldest translation template uploads awaiting review.  The requests should not time out.

Run cronscripts/rosetta-pofile-stats.py.  This will take hours.  Afterwards, POFile translation statistics will still be valid (and not all zeroed out as would happen if the change kicked in inappropriately).


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/model/pofile.py
  lib/lp/translations/model/potemplate.py
-- 
https://code.launchpad.net/~jtv/launchpad/bug-719267/+merge/50651
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-719267 into lp:launchpad.

=== modified file 'lib/lp/translations/model/pofile.py'
--- lib/lp/translations/model/pofile.py	2011-01-26 23:33:20 +0000
+++ lib/lp/translations/model/pofile.py	2011-02-21 20:02:52 +0000
@@ -825,6 +825,12 @@
 
     def _countTranslations(self):
         """Count `currentcount`, `updatescount`, and `rosettacount`."""
+        if self.potemplate.messageCount() == 0:
+            # Shortcut: if the template is empty, as it is when it is
+            # first created, we know the answers without querying the
+            # database.
+            return 0, 0, 0
+
         side_traits = getUtility(ITranslationSideTraitsSet).getForTemplate(
             self.potemplate)
         has_other_msgstrs = "COALESCE(%s) IS NOT NULL" % ", ".join([
@@ -889,6 +895,12 @@
 
     def _countNewSuggestions(self):
         """Count messages with new suggestions."""
+        if self.potemplate.messageCount() == 0:
+            # Shortcut: if the template is empty, as it is when it is
+            # first created, we know the answers without querying the
+            # database.
+            return 0
+
         flag_name = getUtility(ITranslationSideTraitsSet).getForTemplate(
             self.potemplate).flag_name
         suggestion_nonempty = "COALESCE(%s) IS NOT NULL" % ', '.join([

=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py	2011-02-14 17:14:17 +0000
+++ lib/lp/translations/model/potemplate.py	2011-02-21 20:02:52 +0000
@@ -1155,8 +1155,7 @@
             if shared_template is template:
                 continue
             for pofile in shared_template.pofiles:
-                template.newPOFile(
-                    pofile.language.code, False)
+                template.newPOFile(pofile.language.code, create_sharing=False)
             # Do not continue, else it would trigger an existingpo assertion.
             return