Merge lp:~thumper/launchpad/simplistic-official-links-speedup into lp:launchpad
- simplistic-official-links-speedup
- Merge into devel
Status: | Merged |
---|---|
Approved by: | Michael Hudson-Doyle |
Approved revision: | no longer in the source branch. |
Merged at revision: | not available |
Proposed branch: | lp:~thumper/launchpad/simplistic-official-links-speedup |
Merge into: | lp:launchpad |
Diff against target: |
242 lines 4 files modified
lib/lp/code/browser/branchlisting.py (+65/-23) lib/lp/code/browser/tests/test_branchlisting.py (+23/-0) lib/lp/code/interfaces/branch.py (+5/-5) lib/lp/code/model/branch.py (+6/-2) |
To merge this branch: | bzr merge lp:~thumper/launchpad/simplistic-official-links-speedup |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michael Hudson-Doyle | Approve | ||
Review via email: mp+13681@code.launchpad.net |
Commit message
Be more efficient generating the bazaar identities for the branch listings.
Description of the change
Tim Penhey (thumper) wrote : | # |
Michael Hudson-Doyle (mwhudson) wrote : | # |
Generally, I like this branch. It makes the handling of product and
package more consistent, which has to be a good thing. And it reduces
the query counts!
Did you think about adding tests that check how many queries we're issuing?
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -87,6 +87,7 @@
> ProductDownload
> from lp.registry.
> from lp.registry.
> +from lp.registry.
> from lp.registry.
> from lp.registry.
> from lp.registry.
> @@ -153,21 +154,26 @@
> delegates(IBranch, 'context')
>
> def __init__(self, branch, last_commit, now, show_bug_badge,
> - show_blueprint_
> - associated_
> + show_blueprint_
> + associated_
> BranchBadges.
> self.last_commit = last_commit
> self.show_bug_badge = show_bug_badge
> self.show_
> self.show_
> self._now = now
> + self.associated
> + self.suite_
> self.is_
> - self.associated
>
> def associatedProdu
> """Override the IBranch.
> return self.associated
>
> + def associatedSuite
> + """Override the IBranch.
> + return self.suite_
> +
> @property
> def active_
> return [series for series in self.associated
> @@ -314,7 +320,7 @@
> # Requires the following attributes:
> # visible_
> def __init__(self, user):
> - self._dev_
> + self._distro_
> self._now = datetime.
> self.view_user = user
>
> @@ -373,21 +379,63 @@
> series.insert(0, dev_focus)
> return series
>
> - def getDevFocusBran
> - """Get the development focus branch that relates to `branch`."""
> - # XXX: 2009-07-02 TimPenhey spec=package-
> - # here what constitutes a dev focus branch for package branches, and
> - # that perhaps this should also refer to targets instead of using
> - # product.
> - if branch.product is None:
> - return None
> + @cachedproperty
> + def official_
> + """Return a map of ...
Tim Penhey (thumper) wrote : | # |
On Wed, 21 Oct 2009 16:22:06 Michael Hudson wrote:
> Review: Approve
> Generally, I like this branch. It makes the handling of product and
> package more consistent, which has to be a good thing. And it reduces
> the query counts!
>
> Did you think about adding tests that check how many queries we're issuing?
I brought this up at the team lead sprints, and it was somewhat discouraged.
Explicitly checking at some levels about the exact number of queries being
used is ok, but not checking less than a certain level.
Probably better to graph and track query counts on key pages.
> > === modified file 'lib/lp/
> > --- lib/lp/
> > +++ lib/lp/
> > + @cachedproperty
> > + def official_
> > + """Return a map of branch id to a list of package links.
>
> I guess it's a bit hard, English being English, but I found this hard
> to grok. s/of/from/ would help a bit, I think.
Fair enough, changed.
> > + While this code is still valid with package branches is it a
> > query + that isn't needed.
> > + """
>
> Here though I think you have a confusing thinko: s/package/product/
> surely? I'm not quite sure what you mean at a higher level -- are you
> saying that this will be a waste on a page that we know only has
> product branches? In which case an XXX comment would seem more
> appropiate than a mention in the docstring.
>
> Oh, I see: you copied this from product_
> fix up both.
Done.
> > === modified file 'lib/lp/
> > --- lib/lp/
> > +++ lib/lp/
> > @@ -486,6 +486,23 @@
> > login(ANONYMOUS)
> > self.assertBzrI
> >
> > + def test_linked_
> > + # If a branch is linked to the RELEASE pocket of the current
> > + # distroseries for the source package, then the bzr identity is
> > the + # distribution name followed by the sourcepackage name. +
> > branch = self.factory.
> > + pocket = PackagePublishi
> > + linked_branch = ICanHasLinkedBr
> > + branch.
> > + registrant = getUtility(
> > + ILaunchpadCeleb
> > + login_person(
> > + linked_
> > + logout()
> > + login(ANONYMOUS)
> > + self.assertBzrI
> > + branch.
>
> As pointed out in IRC, this test is basically the same as the following.
Yep, deleted.
> Cheers,
> mwh
thanks
Jonathan Lange (jml) wrote : | # |
On Thu, Oct 22, 2009 at 12:57 AM, Tim Penhey <email address hidden> wrote:
> On Wed, 21 Oct 2009 16:22:06 Michael Hudson wrote:
>> Review: Approve
>> Generally, I like this branch. It makes the handling of product and
>> package more consistent, which has to be a good thing. And it reduces
>> the query counts!
>>
>> Did you think about adding tests that check how many queries we're issuing?
>
> I brought this up at the team lead sprints, and it was somewhat discouraged.
>
I don't recall it being discouraged. I just remember Bjorn & Martin
Pool saying to check for equality rather than less than.
jml
Preview Diff
1 | === modified file 'lib/lp/code/browser/branchlisting.py' |
2 | --- lib/lp/code/browser/branchlisting.py 2009-09-22 18:45:02 +0000 |
3 | +++ lib/lp/code/browser/branchlisting.py 2009-10-21 23:58:14 +0000 |
4 | @@ -87,6 +87,7 @@ |
5 | ProductDownloadFileMixin, SortSeriesMixin) |
6 | from lp.registry.interfaces.distroseries import DistroSeriesStatus |
7 | from lp.registry.interfaces.person import IPerson, IPersonSet |
8 | +from lp.registry.interfaces.pocket import PackagePublishingPocket |
9 | from lp.registry.interfaces.product import IProduct |
10 | from lp.registry.interfaces.sourcepackage import ISourcePackageFactory |
11 | from lp.registry.model.sourcepackage import SourcePackage |
12 | @@ -153,21 +154,26 @@ |
13 | delegates(IBranch, 'context') |
14 | |
15 | def __init__(self, branch, last_commit, now, show_bug_badge, |
16 | - show_blueprint_badge, is_dev_focus, |
17 | - associated_product_series, show_mp_badge): |
18 | + show_blueprint_badge, show_mp_badge, |
19 | + associated_product_series, suite_source_packages, is_dev_focus): |
20 | BranchBadges.__init__(self, branch) |
21 | self.last_commit = last_commit |
22 | self.show_bug_badge = show_bug_badge |
23 | self.show_blueprint_badge = show_blueprint_badge |
24 | self.show_merge_proposals = show_mp_badge |
25 | self._now = now |
26 | + self.associated_product_series = associated_product_series |
27 | + self.suite_source_packages = suite_source_packages |
28 | self.is_development_focus = is_dev_focus |
29 | - self.associated_product_series = associated_product_series |
30 | |
31 | def associatedProductSeries(self): |
32 | """Override the IBranch.associatedProductSeries.""" |
33 | return self.associated_product_series |
34 | |
35 | + def associatedSuiteSourcePackages(self): |
36 | + """Override the IBranch.associatedSuiteSourcePackages.""" |
37 | + return self.suite_source_packages |
38 | + |
39 | @property |
40 | def active_series(self): |
41 | return [series for series in self.associated_product_series |
42 | @@ -314,7 +320,7 @@ |
43 | # Requires the following attributes: |
44 | # visible_branches_for_view |
45 | def __init__(self, user): |
46 | - self._dev_series_map = {} |
47 | + self._distro_series_map = {} |
48 | self._now = datetime.now(pytz.UTC) |
49 | self.view_user = user |
50 | |
51 | @@ -345,11 +351,7 @@ |
52 | |
53 | @cachedproperty |
54 | def product_series_map(self): |
55 | - """Return a map of branch id to a list of product series. |
56 | - |
57 | - While this code is still valid with package branches is it a query |
58 | - that isn't needed. |
59 | - """ |
60 | + """Return a map from branch id to a list of product series.""" |
61 | series_resultset = self._query_optimiser.getProductSeriesForBranches( |
62 | self._visible_branch_ids) |
63 | result = {} |
64 | @@ -373,21 +375,59 @@ |
65 | series.insert(0, dev_focus) |
66 | return series |
67 | |
68 | - def getDevFocusBranch(self, branch): |
69 | - """Get the development focus branch that relates to `branch`.""" |
70 | - # XXX: 2009-07-02 TimPenhey spec=package-branches We need to determine |
71 | - # here what constitutes a dev focus branch for package branches, and |
72 | - # that perhaps this should also refer to targets instead of using |
73 | - # product. |
74 | - if branch.product is None: |
75 | - return None |
76 | + @cachedproperty |
77 | + def official_package_links_map(self): |
78 | + """Return a map from branch id to a list of package links.""" |
79 | + links = self._query_optimiser.getOfficialSourcePackageLinksForBranches( |
80 | + self._visible_branch_ids) |
81 | + result = {} |
82 | + for link in links: |
83 | + result.setdefault(link.branch.id, []).append(link) |
84 | + return result |
85 | + |
86 | + def getSuiteSourcePackages(self, branch): |
87 | + """Get the associated SuiteSourcePackages for the branch. |
88 | + |
89 | + If there is more than one, they are sorted by pocket. |
90 | + """ |
91 | + links = [link.suite_sourcepackage for link in |
92 | + self.official_package_links_map.get(branch.id, [])] |
93 | + return sorted(links, key=attrgetter('pocket')) |
94 | + |
95 | + def getDistroDevelSeries(self, distribution): |
96 | + """Since distribution.currentseries hits the DB every time, cache it.""" |
97 | + self._distro_series_map = {} |
98 | try: |
99 | - return self._dev_series_map[branch.product] |
100 | + return self._distro_series_map[distribution] |
101 | except KeyError: |
102 | - result = branch.product.development_focus.branch |
103 | - self._dev_series_map[branch.product] = result |
104 | + result = distribution.currentseries |
105 | + self._distro_series_map[distribution] = result |
106 | return result |
107 | |
108 | + def isBranchDevFocus(self, branch, |
109 | + associated_product_series, suite_source_packages): |
110 | + """Is the branch the development focus? |
111 | + |
112 | + For product branches this means that the branch is linked to the |
113 | + development focus series. |
114 | + |
115 | + For package branches this means that the branch is linked to the |
116 | + release pocket of the development series. |
117 | + """ |
118 | + # Refactor this code to work for model.branch too? |
119 | + # Do we care if a non-product branch is linked to the product series? |
120 | + # Do we care if a non-package branch is linked to the package? |
121 | + # A) not right now. |
122 | + for series in associated_product_series: |
123 | + if series.product.development_focus == series: |
124 | + return True |
125 | + for ssp in suite_source_packages: |
126 | + if (ssp.pocket == PackagePublishingPocket.RELEASE and |
127 | + ssp.distroseries == self.getDistroDevelSeries( |
128 | + ssp.distribution)): |
129 | + return True |
130 | + return False |
131 | + |
132 | @cachedproperty |
133 | def branch_ids_with_bug_links(self): |
134 | """Return a set of branch ids that should show bug badges.""" |
135 | @@ -436,11 +476,13 @@ |
136 | show_blueprint_badge = branch.id in self.branch_ids_with_spec_links |
137 | show_mp_badge = branch.id in self.branch_ids_with_merge_proposals |
138 | associated_product_series = self.getProductSeries(branch) |
139 | - is_dev_focus = (self.getDevFocusBranch(branch) == branch) |
140 | + suite_source_packages = self.getSuiteSourcePackages(branch) |
141 | + is_dev_focus = self.isBranchDevFocus( |
142 | + branch, associated_product_series, suite_source_packages) |
143 | return BranchListingItem( |
144 | branch, last_commit, self._now, show_bug_badge, |
145 | - show_blueprint_badge, is_dev_focus, |
146 | - associated_product_series, show_mp_badge) |
147 | + show_blueprint_badge, show_mp_badge, |
148 | + associated_product_series, suite_source_packages, is_dev_focus) |
149 | |
150 | def decoratedBranches(self, branches): |
151 | """Return the decorated branches for the branches passed in.""" |
152 | |
153 | === modified file 'lib/lp/code/browser/tests/test_branchlisting.py' |
154 | --- lib/lp/code/browser/tests/test_branchlisting.py 2009-10-01 11:21:31 +0000 |
155 | +++ lib/lp/code/browser/tests/test_branchlisting.py 2009-10-21 23:58:14 +0000 |
156 | @@ -307,6 +307,29 @@ |
157 | self.assertGroupBranchesEqual(expected, series) |
158 | |
159 | |
160 | +class TestDevelopmentFocusPackageBranches(TestCaseWithFactory): |
161 | + """Make sure that the bzr_identity of the branches are correct.""" |
162 | + |
163 | + layer = DatabaseFunctionalLayer |
164 | + |
165 | + def test_package_development_focus(self): |
166 | + # Check the bzr_identity of a development focus package branch. |
167 | + branch = self.factory.makePackageBranch() |
168 | + series_set = removeSecurityProxy(getUtility(IMakeOfficialBranchLinks)) |
169 | + sspb = series_set.new( |
170 | + branch.distroseries, PackagePublishingPocket.RELEASE, |
171 | + branch.sourcepackagename, branch, branch.owner) |
172 | + identity = "lp://dev/%s/%s" % ( |
173 | + branch.distribution.name, branch.sourcepackagename.name) |
174 | + self.assertEqual(identity, branch.bzr_identity) |
175 | + # Now confirm that we get the same through the view. |
176 | + view = create_initialized_view(branch.distribution, name='+branches') |
177 | + # There is only one branch. |
178 | + batch = view.branches() |
179 | + [view_branch] = batch.branches |
180 | + self.assertEqual(identity, view_branch.bzr_identity) |
181 | + |
182 | + |
183 | def test_suite(): |
184 | return unittest.TestLoader().loadTestsFromName(__name__) |
185 | |
186 | |
187 | === modified file 'lib/lp/code/interfaces/branch.py' |
188 | --- lib/lp/code/interfaces/branch.py 2009-10-15 05:45:57 +0000 |
189 | +++ lib/lp/code/interfaces/branch.py 2009-10-21 23:58:14 +0000 |
190 | @@ -68,7 +68,6 @@ |
191 | from lp.code.interfaces.branchlookup import IBranchLookup |
192 | from lp.code.interfaces.branchtarget import IHasBranchTarget |
193 | from lp.code.interfaces.hasbranches import IHasMergeProposals |
194 | -from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch |
195 | from canonical.launchpad.interfaces.launchpad import ( |
196 | ILaunchpadCelebrities, IPrivacy) |
197 | from lp.registry.interfaces.role import IHasOwner |
198 | @@ -1230,10 +1229,11 @@ |
199 | 'series': use_series.name} |
200 | |
201 | if branch.sourcepackage is not None: |
202 | - distro_package = branch.sourcepackage.distribution_sourcepackage |
203 | - linked_branch = ICanHasLinkedBranch(distro_package) |
204 | - if linked_branch.branch == branch: |
205 | - return lp_prefix + linked_branch.bzr_path |
206 | + if is_dev_focus: |
207 | + return "%(prefix)s%(distro)s/%(packagename)s" % { |
208 | + 'prefix': lp_prefix, |
209 | + 'distro': branch.distroseries.distribution.name, |
210 | + 'packagename': branch.sourcepackagename.name} |
211 | suite_sourcepackages = branch.associatedSuiteSourcePackages() |
212 | # Take the first link if there is one. |
213 | if len(suite_sourcepackages) > 0: |
214 | |
215 | === modified file 'lib/lp/code/model/branch.py' |
216 | --- lib/lp/code/model/branch.py 2009-10-06 16:24:20 +0000 |
217 | +++ lib/lp/code/model/branch.py 2009-10-21 23:58:14 +0000 |
218 | @@ -68,6 +68,7 @@ |
219 | from lp.code.interfaces.branchnamespace import IBranchNamespacePolicy |
220 | from lp.code.interfaces.branchpuller import IBranchPuller |
221 | from lp.code.interfaces.branchtarget import IBranchTarget |
222 | +from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch |
223 | from lp.code.interfaces.seriessourcepackagebranch import ( |
224 | IFindOfficialBranchLinks) |
225 | from lp.registry.interfaces.person import ( |
226 | @@ -441,11 +442,14 @@ |
227 | @property |
228 | def bzr_identity(self): |
229 | """See `IBranch`.""" |
230 | - # XXX: JonathanLange 2009-03-19 spec=package-branches bug=345740: This |
231 | - # should not dispatch on product is None. |
232 | + # Should probably put this into the branch target. |
233 | if self.product is not None: |
234 | series_branch = self.product.development_focus.branch |
235 | is_dev_focus = (series_branch == self) |
236 | + elif self.distroseries is not None: |
237 | + distro_package = self.sourcepackage.distribution_sourcepackage |
238 | + linked_branch = ICanHasLinkedBranch(distro_package) |
239 | + is_dev_focus = (linked_branch.branch == self) |
240 | else: |
241 | is_dev_focus = False |
242 | return bazaar_identity(self, is_dev_focus) |
= Summary =
This branch reduces the query count on branch listings that contain package
branches.
== Proposed fix ==
Get the branch listing browser classes to use the query optimiser to load all
the suite source package links for the branches, and make those available
through the decorated Branch class.
Move the package development focus check into the model code for branch, and
change the bazaar_identity function to us is_dev_focus for package branches.
Also determine the development focus in the browser class.
== Pre-implementation notes ==
Distribution. currentseries is an abomination and had to be worked around in
the browser class.
== Implementation details ==
I've deferred some possible refactoring of the dev focus checks.
== Tests ==
test_linked_ to_development_ package FocusPackageBra nches
TestDevelopment
code/stories
TestBzrIdentity
== Demo and Q/A ==
Add the mint distro using the code test helper, and go to look at: /code.launchpad .dev/mint/ +branches? batch=150
https:/
and look for lp://dev/ mint/twisted
reviewer mwhudson