Merge lp:~sinzui/launchpad/cache-tweaks-0 into lp:launchpad

[Curtis Hovey]

This is my branch to adjust the cache rules for pages.

    lp:~sinzui/launchpad/cache-tweaks-0
    Diff size: 357
    Launchpad bug: https://bugs.launchpad.net/bugs/586058
    Test command: ./bin/test -vv \
        -t milestone -t test_hasspecifications
    Pre-implementation: no one, I really am guessing, really
    Target release: 10.05

Adjust the cache rules for pages
---------------------------------

Edge timeout reports a report a reduction of 50%-80% in timeouts in the pages
that were added. Timeouts still happen for all cached pages though. In the
case of milestones, the caching is too long. Release managers need to see
milestone changes; consider using different cache rules for active milestones.

Rules
-----

Look at the timeouts on edge reported in in the oopses. Identify the slow
chunks.

QA
--

Verify distroseries, project and project group milestone pages
verify the launchpad team spec workload page loads.
verify maverick loads

Lint
----

Linting changed files:
  lib/lp/answers/templates/faqcollection-portlet-faqs.pt
  lib/lp/blueprints/browser/tests/test_hasspecifications.py
  lib/lp/registry/browser/configure.zcml
  lib/lp/registry/browser/milestone.py
  lib/lp/registry/browser/person.py
  lib/lp/registry/browser/tests/milestone-views.txt
  lib/lp/registry/templates/distroseries-portlet-packaging.pt
  lib/lp/registry/templates/milestone-index.pt
  lib/lp/registry/templates/milestone-macros.pt
  lib/lp/registry/templates/productseries-milestone-table-row.pt
  lib/lp/soyuz/templates/distroseries-portlet-architectures.pt

Test
----

    * lib/lp/blueprints/browser/tests/test_hasspecifications.py
      * Added a rudimentary test for the batch size change.
    * lib/lp/registry/browser/tests/milestone-views.txt
      * Added a simple test to verify that the milestone bugtask cache rules
        are based on the state of the milestone.

Implementation
--------------

    * lib/lp/answers/templates/faqcollection-portlet-faqs.pt
      * Increased expiration age because FAQs do not change every day. This
        may help project and project group pages.
    * lib/lp/registry/browser/configure.zcml
      * Registered +milestone-macros so that the milestone bugtask table
        can have different cache times.
    * lib/lp/registry/browser/milestone.py
      * Assembled a menu that is fast to use (no subscription link) when
        working with many milestones. Many pages iterate over milestones and
        provide links to work with them in the list. The oopses indicate the
        timeouts happen when setting up the bug subscription link that is not
        used!
    * lib/lp/registry/browser/person.py
      * Decreased the TestPersonSpecWorkloadView.member batch size from
        75 to 20. I *hope* this fixes the issue on edge. I really cannot
        see if the issue is members or the number of specs, but I know the
        team in the oops is ~launchpad. I think this oops is ironic since
        the launchpad team abandoned blueprints.
    * lib/lp/registry/templates/distroseries-portlet-packaging.pt
      * This is still a common cause of oopses. Until the queries can be made
        faster, the cache expiration age should be increased to 3 hours.
    * lib/lp/registry/templates/milestone-index.pt
      * Use a menu without the subscription link when working with inline
        links to avoid repetitive db look ups.
      * Extracted the bugtask table to a macro so that the cache time can
        be different for active milestones. We are getting timeouts on lucid
        milestone pages are are inactive--they cannot change. 1 hour
        expiration age is too long for release managers that are working with
        active milestones. This invasive refactoring permits active milestones
        to cache for 10 minutes and inactive one to cache for 3 hours. This
        helps distro, project and project group milestones.
    * lib/lp/registry/templates/milestone-macros.pt
      * Extracted the milestone bugtask table and wrapped it in a macro for
        reuse.
    * lib/lp/registry/templates/productseries-milestone-table-row.pt
      * Use a fast menu when creating milestone links. This helps the project
        and distro series pages as well as the +series page.
    * lib/lp/soyuz/templates/distroseries-portlet-architectures.pt
      * Architectures do not change after they are set, so cache the portlet
        longer. This may help the distroseries page.

[Jelmer Vernooij (jelmer)]

Thanks!

My tal knowledge is still pretty basic so it would be great if somebody else with more tal knowledge could also have a look; I understand what's happening but I'm probably not aware of all of the caveats.

review: Approve (code*)

[Michael Nelson (michael.nelson)]

I can look at this tomorrow, but have to run right now... so assuming you're keen to land this now, can you please request a review from someone else? Thanks.

review: Abstain (code)

[Gary Poster (gary)]

Thank you, Curtis. This sounds like a big improvement, and looks very cleanly done.

Notes:

- The testing of the cache changes--looking for comments that duplicate your cache settings--is not ideal. Could you create a bug for foundations giving an example of the kinds of tests you were trying to write, requesting a better testing pattern? Vaguely and naively, I would think we ought to have some test cache available that lets you ask questions about what is cached and for how long. The Foundations deliverable for the bug perhaps should be a refactoring of one or more of your tests, to show that our changes address the problem.

- Similarly, the fact that you had to create a macro simply to conditionally change the wrapping cache settings is broken. I'm fine with this as a temporary solution, but I'd like Foundations to provide a nicer spelling, or at least be challenged to do so. That said, nothing comes to mind initially. :-/ But anyway, a bug is probably appropriate. We can always promote a technique (see next bullet point) rather than changing the syntax, if that's what we decide is the better solution.

- I'm pretty confident it would be possible to put the macro in the template itself, so that you didn't have to make a separate file for it. That's the way I would have tried to do it, since this macro is only used locally. You could have used define-macro within the first block, and then use-macro in the second block. I forget how to address the local macros: it might be template/macros/NAME_OF_LOCAL_MACRO. I suspect you just want to get this in before PQM closes this evening (and maybe you'd want to argue the style), so I'm OK with you not pursuing this. Anyway, here's my untested example:

        <tal:has_bugtasks condition="bugtasks">
          <tal:active content="cache:private, 10 minute"
            condition="view/milestone/active">
            <!-- milestone_bugtasks cache:private 10 minute -->
            <tal:milestone-bugtasks
              metal:define-macro="milestone_bugtasks">
            IMPLEMENTATION GOO GOES HERE
            </tal:milestone-bugtasks>
          </tal:active>
          <tal:inactive content="cache:private, 3 hour"
            condition="not: view/milestone/active">
            <!-- milestone_bugtasks cache:private 3 hour -->
            <tal:milestone-bugtasks
              metal:use-macro="template/macros/milestone_bugtasks" />
          </tal:inactive>
        </tal:has_bugtasks>

review: Approve

[Curtis Hovey (sinzui)]

On Thu, 2010-05-27 at 16:00 +0000, Gary Poster wrote:
> Review: Approve
> Thank you, Curtis. This sounds like a big improvement, and looks very cleanly done.
>
> Notes:
>
> - The testing of the cache changes--looking for comments that
> duplicate your cache settings--is not ideal.

I reported [Testing tal cache states could be easier]
https://bugs.edge.launchpad.net/launchpad-foundations/+bug/586466

> - Similarly, the fact that you had to create a macro simply to
> conditionally change the wrapping cache settings is broken.

I reported [tal cache: directive could support an age variable]
https://bugs.edge.launchpad.net/launchpad-foundations/+bug/586461

> - I'm pretty confident it would be possible to put the macro in the
> template itself, so that you didn't have to make a separate file for
> it. That's the way I would have tried to do it, since this macro is
> only used locally. You could have used define-macro within the first
> block, and then use-macro in the second block. I forget how to
> address the local macros: it might be
> template/macros/NAME_OF_LOCAL_MACRO. I suspect you just want to get
> this in before PQM closes this evening (and maybe you'd want to argue
> the style), so I'm OK with you not pursuing this. Anyway, here's my
> untested example:
>
>
> <tal:has_bugtasks condition="bugtasks">
> <tal:active content="cache:private, 10 minute"
> condition="view/milestone/active">
> <!-- milestone_bugtasks cache:private 10 minute -->
> <tal:milestone-bugtasks
> metal:define-macro="milestone_bugtasks">
> IMPLEMENTATION GOO GOES HERE
> </tal:milestone-bugtasks>
> </tal:active>
> <tal:inactive content="cache:private, 3 hour"
> condition="not: view/milestone/active">
> <!-- milestone_bugtasks cache:private 3 hour -->
> <tal:milestone-bugtasks
> metal:use-macro="template/macros/milestone_bugtasks" />
> </tal:inactive>
> </tal:has_bugtasks>

I tried something similar, but it was getting test errors in some
conditions. I'll keep the macro for now. If we can get the variable age
issue solved I could remove the macro.

--
__Curtis C. Hovey_________
http://launchpad.net/

[Robert Collins (lifeless)]

I suggest invalidating pages as backend content changes - this is what
yahoo do for their news site - and they have more volume than we do :)
I'm happy to chat about how anytime you like - or even arrange some
time with Mark Nottingham to detail what they do.

[Curtis Hovey (sinzui)]

We do not have an api to invalid all cached junks that involve an object. Having one would really simplify situations where users update bugs that are linked to a milestone. The automatic key generation needs rethinking.

=== modified file 'lib/lp/answers/templates/faqcollection-portlet-faqs.pt'
--- lib/lp/answers/templates/faqcollection-portlet-faqs.pt	2010-05-19 14:20:49 +0000
+++ lib/lp/answers/templates/faqcollection-portlet-faqs.pt	2010-05-27 20:02:31 +0000
@@ -18,7 +18,7 @@
   </form>
 
   <ul tal:condition="faqs"
-    tal:content="cache:public, 1 hour">
+    tal:content="cache:public, 3 hour">
     <li tal:repeat="faq faqs">
       <a class="sprite question"
         tal:attributes="href faq/fmt:url"

=== added file 'lib/lp/blueprints/browser/tests/test_hasspecifications.py'
--- lib/lp/blueprints/browser/tests/test_hasspecifications.py	1970-01-01 00:00:00 +0000
+++ lib/lp/blueprints/browser/tests/test_hasspecifications.py	2010-05-27 20:02:31 +0000
@@ -0,0 +1,34 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import unittest
+
+from lp.testing import login_person, TestCaseWithFactory
+from lp.testing.views import create_initialized_view
+from canonical.testing import DatabaseFunctionalLayer
+
+
+class TestPersonSpecWorkloadView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestPersonSpecWorkloadView, self).setUp()
+        self.owner = self.factory.makePerson(name='blue')
+        login_person(self.owner)
+        self.team = self.factory.makeTeam(name='square', owner='blue')
+        self.member = self.factory.makePerson(name='green')
+        self.team.addMember(self.member, self.owner)
+
+    def test_view_attributes(self):
+        view = create_initialized_view(
+            self.team, name='+specworkload')
+        label = 'Blueprint workload'
+        self.assertEqual(label, view.label)
+        self.assertEqual(20, view.members.batch.size)
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2010-05-18 22:36:20 +0000
+++ lib/lp/registry/browser/configure.zcml	2010-05-27 20:02:31 +0000
@@ -1258,6 +1258,11 @@
         path_expression="string:+milestone/${name}"
         rootsite="mainsite"
         attribute_to_parent="target"/>
+    <browser:page
+        for="*"
+        name="+milestone-macros"
+        permission="zope.Public"
+        template="../templates/milestone-macros.pt"/>
     <browser:pages
         for="lp.registry.interfaces.milestone.IMilestone"
         class="lp.registry.browser.milestone.MilestoneView"

=== modified file 'lib/lp/registry/browser/milestone.py'
--- lib/lp/registry/browser/milestone.py	2010-05-14 19:06:48 +0000
+++ lib/lp/registry/browser/milestone.py	2010-05-27 20:02:31 +0000
@@ -130,9 +130,11 @@
 
 class MilestoneOverviewMenu(ApplicationMenu, MilestoneLinkMixin):
     """Overview  menus for `IMilestone` objects."""
+    # This menu must not contain 'subscribe' because the link state is too
+    # costly to calculate when this menu is used with a list of milestones.
     usedfor = IMilestone
     facet = 'overview'
-    links = ('create_release', )
+    links = ('edit', 'create_release')
 
 
 class IMilestoneInline(Interface):

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-05-27 07:02:09 +0000
+++ lib/lp/registry/browser/person.py	2010-05-27 20:02:31 +0000
@@ -1733,7 +1733,7 @@
         assert self.context.isTeam, (
             "PersonSpecWorkloadView.members can only be called on a team.")
         members = self.context.allmembers
-        batch_nav = BatchNavigator(members, self.request)
+        batch_nav = BatchNavigator(members, self.request, size=20)
         return batch_nav
 
 

=== modified file 'lib/lp/registry/browser/tests/milestone-views.txt'
--- lib/lp/registry/browser/tests/milestone-views.txt	2010-05-17 13:07:31 +0000
+++ lib/lp/registry/browser/tests/milestone-views.txt	2010-05-27 20:02:31 +0000
@@ -301,6 +301,29 @@
     >>> login_person(person)
 
 
+Milestone caching
+-----------------
+
+Active milestones cache bugtasks for 10 minutes.
+
+    >>> cached_milestone = factory.makeMilestone(
+    ...     'cache-test', productseries=series)
+    >>> cached_bugtask = factory.makeBugTask(target=cached_milestone.product)
+    >>> cached_bugtask.milestone = cached_milestone
+    >>> view = create_view(cached_milestone, '+index', principal=person)
+    >>> content = view.render()
+    >>> 'milestone_bugtasks cache:private 10 minute' in content
+    True
+
+Inactive milestones cache bugtasks for 3 hours.
+
+    >>> cached_milestone.active = False
+    >>> view = create_view(cached_milestone, '+index', principal=person)
+    >>> content = view.render()
+    >>> 'milestone_bugtasks cache:private 3 hour' in content
+    True
+
+
 ProjectGroup milestones
 -----------------------
 

=== modified file 'lib/lp/registry/templates/distroseries-portlet-packaging.pt'
--- lib/lp/registry/templates/distroseries-portlet-packaging.pt	2010-05-17 17:29:08 +0000
+++ lib/lp/registry/templates/distroseries-portlet-packaging.pt	2010-05-27 20:02:31 +0000
@@ -4,7 +4,7 @@
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
   id="series-packaging" class="portlet"
   tal:define="overview_menu context/menu:overview"
-  tal:content="cache:public, 1 hour">
+  tal:content="cache:public, 3 hour">
   <h2>Upstream packaging</h2>
 
   <p id="packaging-summary"

=== modified file 'lib/lp/registry/templates/milestone-index.pt'
--- lib/lp/registry/templates/milestone-index.pt	2010-05-17 17:29:08 +0000
+++ lib/lp/registry/templates/milestone-index.pt	2010-05-27 20:02:31 +0000
@@ -24,6 +24,7 @@
       specs view/specifications;
       bugtasks view/bugtasks;
       release_menu view/release/menu:context|Nothing;
+      milestone_menu view/milestone/menu:overview;
       has_edit context/required:launchpad.Edit">
 
       <div class="top-portlet">
@@ -73,7 +74,7 @@
                   tal:attributes="title context/dateexpected/fmt:datetime"
                   tal:content="context/dateexpected/fmt:approximatedate" />
                   &nbsp;
-                  <a tal:replace="structure view/milestone/menu:overview/create_release/fmt:icon-link" />
+                  <a tal:replace="structure milestone_menu/create_release/fmt:icon-link" />
                 </dd>
               </dl>
 
@@ -112,7 +113,7 @@
                 <tal:no condition="not: context/active">
                   No.  Drivers cannot target bugs and blueprints to this milestone.
                 </tal:no>
-                &nbsp; <a tal:replace="structure view/milestone/menu:navigation/edit/fmt:icon" />
+                &nbsp; <a tal:replace="structure milestone_menu/edit/fmt:icon" />
               </dd>
             </dl>
 
@@ -264,63 +265,19 @@
           </table>
         </tal:has_specs>
 
-        <tal:has_bugtasks condition="bugtasks"
-          tal:content="cache:private, 1 hour">
-          <table class="listing sortable" id="milestone_bugtasks">
-            <thead>
-              <tr>
-                <th colspan="4">Bug report</th>
-                <th style="width: 14em"
-                  tal:condition="view/is_project_milestone">
-                  Project
-                </th>
-                <th style="width: 9em;">Importance</th>
-                <th style="width: 14em;">Assignee</th>
-                <th style="width: 10em;">Status</th>
-              </tr>
-            </thead>
-            <tbody>
-              <tr tal:repeat="bugtask bugtasks">
-                <td class="icon left">
-                  <span class="sortkey" tal:content="bugtask/bug/id" />
-                  <span tal:content="structure bugtask/image:icon" />
-                </td>
-                <td tal:content="string:#${bugtask/bug/id}" class="amount">12345</td>
-                <td>
-                  <a tal:attributes="href bugtask/bug/fmt:url"
-                     tal:content="bugtask/bug/title">
-                     Bug Title Here
-                  </a>
-                </td>
-                <td style="padding-right: 5px; text-align:right">
-                  <img src="/@@/info" tal:replace="structure bugtask/image:badges" />
-                </td>
-                <td tal:condition="view/is_project_milestone">
-                  <span class="sortkey" tal:content="bugtask/target/displayname" />
-                  <a tal:attributes="href bugtask/milestone/fmt:url"
-                     tal:content="bugtask/target/displayname" />
-                </td>
-                <td>
-                  <span class="sortkey" tal:content="bugtask/importance/sortkey" />
-                  <span tal:content="bugtask/importance/title"
-                        tal:attributes="
-                        class string:importance${bugtask/importance/name}">High</span>
-                </td>
-                <td>
-                  <a tal:condition="bugtask/assignee"
-                     tal:replace="structure bugtask/assignee/fmt:link" />&nbsp;
-                </td>
-                <td
-                  tal:attributes="title bugtask/last_significant_change_date/fmt:displaydate">
-                  <span class="sortkey" tal:content="bugtask/status/sortkey" />
-                  <span
-                      tal:content="bugtask/status/title"
-                      tal:attributes="
-                      class string:status${bugtask/status/name}">High</span>
-                </td>
-              </tr>
-            </tbody>
-          </table>
+        <tal:has_bugtasks condition="bugtasks">
+          <tal:active content="cache:private, 10 minute"
+            condition="view/milestone/active">
+            <!-- milestone_bugtasks cache:private 10 minute -->
+            <tal:milestone-bugtasks
+              metal:use-macro="context/@@+milestone-macros/milestone_bugtasks" />
+          </tal:active>
+          <tal:inactive content="cache:private, 3 hour"
+            condition="not: view/milestone/active">
+            <!-- milestone_bugtasks cache:private 3 hour -->
+            <tal:milestone-bugtasks
+              metal:use-macro="context/@@+milestone-macros/milestone_bugtasks" />
+          </tal:inactive>
         </tal:has_bugtasks>
 
         <tal:neither_bugs_nor_specs condition="not: view/has_bugs_or_specs">

=== added file 'lib/lp/registry/templates/milestone-macros.pt'
--- lib/lp/registry/templates/milestone-macros.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/templates/milestone-macros.pt	2010-05-27 20:02:31 +0000
@@ -0,0 +1,65 @@
+<tal:root
+  xmlns:tal="http://xml.zope.org/namespaces/tal"
+  xmlns:metal="http://xml.zope.org/namespaces/metal"
+  omit-tag="">
+
+
+
+  <tal:milestone-bugtasks metal:define-macro="milestone_bugtasks">
+    <table class="listing sortable" id="milestone_bugtasks">
+      <thead>
+        <tr>
+          <th colspan="4">Bug report</th>
+          <th style="width: 14em"
+            tal:condition="view/is_project_milestone">
+            Project
+          </th>
+          <th style="width: 9em;">Importance</th>
+          <th style="width: 14em;">Assignee</th>
+          <th style="width: 10em;">Status</th>
+        </tr>
+      </thead>
+      <tbody>
+        <tr tal:repeat="bugtask bugtasks">
+          <td class="icon left">
+            <span class="sortkey" tal:content="bugtask/bug/id" />
+            <span tal:content="structure bugtask/image:icon" />
+          </td>
+          <td tal:content="string:#${bugtask/bug/id}" class="amount">12345</td>
+          <td>
+            <a tal:attributes="href bugtask/bug/fmt:url"
+               tal:content="bugtask/bug/title">
+               Bug Title Here
+            </a>
+          </td>
+          <td style="padding-right: 5px; text-align:right">
+            <img src="/@@/info" tal:replace="structure bugtask/image:badges" />
+          </td>
+          <td tal:condition="view/is_project_milestone">
+            <span class="sortkey" tal:content="bugtask/target/displayname" />
+            <a tal:attributes="href bugtask/milestone/fmt:url"
+               tal:content="bugtask/target/displayname" />
+          </td>
+          <td>
+            <span class="sortkey" tal:content="bugtask/importance/sortkey" />
+            <span tal:content="bugtask/importance/title"
+                  tal:attributes="
+                  class string:importance${bugtask/importance/name}">High</span>
+          </td>
+          <td>
+            <a tal:condition="bugtask/assignee"
+               tal:replace="structure bugtask/assignee/fmt:link" />&nbsp;
+          </td>
+          <td
+            tal:attributes="title bugtask/last_significant_change_date/fmt:displaydate">
+            <span class="sortkey" tal:content="bugtask/status/sortkey" />
+            <span
+                tal:content="bugtask/status/title"
+                tal:attributes="
+                class string:status${bugtask/status/name}">High</span>
+          </td>
+        </tr>
+      </tbody>
+    </table>
+  </tal:milestone-bugtasks>
+</tal:root>

=== modified file 'lib/lp/registry/templates/productseries-milestone-table-row.pt'
--- lib/lp/registry/templates/productseries-milestone-table-row.pt	2010-05-17 20:09:03 +0000
+++ lib/lp/registry/templates/productseries-milestone-table-row.pt	2010-05-27 20:02:31 +0000
@@ -2,7 +2,7 @@
   xmlns:tal="http://xml.zope.org/namespaces/tal"
   xmlns:metal="http://xml.zope.org/namespaces/metal"
   xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  define="milestone_menu view/menu:navigation;
+  define="milestone_menu view/milestone/menu:overview;
           milestone view/milestone;
           release view/release"
   tal:content="cache:private, 1 hour">
@@ -59,7 +59,7 @@
           not yet released
         </tal:cannot-edit>
         <tal:can-edit
-          define="link view/milestone/menu:overview/create_release"
+          define="link milestone_menu/create_release"
           condition="link/enabled">
           <a style="white-space: nowrap"
             tal:attributes="href link/fmt:url;

=== modified file 'lib/lp/soyuz/templates/distroseries-portlet-architectures.pt'
--- lib/lp/soyuz/templates/distroseries-portlet-architectures.pt	2010-05-17 17:29:08 +0000
+++ lib/lp/soyuz/templates/distroseries-portlet-architectures.pt	2010-05-27 20:02:31 +0000
@@ -14,7 +14,7 @@
 
   <tal:buildable condition="architectures">
     <ul id="portlet-architectures-list"
-      tal:content="cache:public, 1 hour">
+      tal:content="cache:public, 3 hour">
       <tal:official repeat="architecture architectures">
         <li tal:condition="architecture/official">
           <a class="sprite architecture"