Launchpad itself

Merge lp:~wgrant/launchpad/bprdc into lp:launchpad/db-devel

  1. bprdc
  2. Merge into db-devel
Proposed by William Grant
Status: Merged
Approved by: Henning Eggers
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~wgrant/launchpad/bprdc
Merge into: lp:launchpad/db-devel
Prerequisite: lp:~wgrant/launchpad/get-bpr-by-filename
Diff against target: 343 lines (+204/-7)
7 files modified
database/schema/patch-2207-36-0.sql (+15/-0)
database/schema/security.cfg (+1/-0)
lib/lp/soyuz/interfaces/archive.py (+14/-0)
lib/lp/soyuz/interfaces/binarypackagerelease.py (+23/-4)
lib/lp/soyuz/model/archive.py (+15/-0)
lib/lp/soyuz/model/binarypackagerelease.py (+33/-2)
lib/lp/soyuz/tests/test_archive.py (+103/-1)
To merge this branch: bzr merge lp:~wgrant/launchpad/bprdc
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve
Henning Eggers (community) code Approve
Björn Tillenius (community) db Approve
Stuart Bishop (community) db Approve
Review via email: mp+21213@code.launchpad.net

Commit message

Add the model for PPA download stats. Landed by henninge.

Description of the change

This branch adds the model for PPA binary download stats. BinaryPackageReleaseDownloadCount is the class and DB table, with a new Archive.updatePackageDownloadCount to create and update them. See bug #139855 for discussion on the issue -- the only thing with which everybody is agreed about collecting is counts for each binary package release in each archive.

BinaryPackageReleaseDownloadCounts will be managed by my next few branches, with the parse-ppa-apache-logs.py script. Otherwise very closely modeled on LibraryFileDownloadCount, it will store counts with an (archive, bpr) key rather than just (lfa,).

To post a comment you must log in.
Revision history for this message
Stuart Bishop (stub) wrote :

Looks good. DB patch approved by me as patch-2207-36-0.sql.

We might need an extra index or three depending on what reports we generate from this information. I think the reports I think we will generate will be fine with the index from the UNIQUE constraint so no worries for now.

review: Approve (db)
Revision history for this message
Björn Tillenius (bjornt) wrote :

On Fri, Mar 12, 2010 at 07:20:53AM -0000, William Grant wrote:
> William Grant has proposed merging lp:~wgrant/launchpad/bprdc into lp:launchpad with lp:~wgrant/launchpad/get-bpr-by-filename as a prerequisite.
>
> Requested reviews:
> Björn Tillenius (bjornt): db
> Stuart Bishop (stub): db
> Canonical Launchpad Engineering (launchpad)
>
>
> This branch adds the model for PPA binary download stats.
> BinaryPackageReleaseDownloadCount is the class and DB table, with a new
> Archive.updatePackageDownloadCount to create and update them. See bug
> #139855 for discussion on the issue -- the only thing with which
> everybody is agreed about collecting is counts for each binary package
> release in each archive.

I think this patch is ok, although I have a few questions (and a comment
on the patch itself further down). I'll be on vacation next week, so I
won't block a landing of the patch, waiting for the replies, as long as
my comments are considered, but I'm marking it as needsinfo so that I
can track the discussion more easily.

    vote needsinfo

Can you explain shortly how BinaryPackageRelease relates to archive and
series? As I understand it, this is only for PPAs, right, since we can't
track the downloads of other archives? Would it make sense to make the
name a bit more specific in that case? At the very least, comments.sql
should explain this. Or will this be used by other archives than PPA
ones?

I couldn't find where everybody agreed about collecting counts for each
binary package release. Can you point me to the right place?

What other things are we likely to want to track in the future, if any?

> === added file 'database/schema/patch-2207-99-0.sql'
> --- database/schema/patch-2207-99-0.sql 1970-01-01 00:00:00 +0000
> +++ database/schema/patch-2207-99-0.sql 2010-03-12 07:20:45 +0000
> @@ -0,0 +1,15 @@
> +SET client_min_messages=ERROR;
> +
> +CREATE TABLE BinaryPackageReleaseDownloadCount (
> + id serial PRIMARY KEY,
> + archive integer NOT NULL REFERENCES Archive,
> + binary_package_release integer NOT NULL REFERENCES BinaryPackageRelease,
> + day date NOT NULL,
> + country integer,
> + count integer NOT NULL
> +);
> +
> +ALTER TABLE BinaryPackageReleaseDownloadCount ADD CONSTRAINT binarypackagereleasedownloadcount__archive__binary_package_release__day__country__key
> + UNIQUE (archive, binary_package_release, day, country);
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 99, 0);

Shouldn't country be a foreign key to Country?

And looking at the tests, I didn't see any tests for not specifying the
country. That would be good to have.

--
Björn Tillenius | https://launchpad.net/~bjornt

review: Needs Information
Revision history for this message
William Grant (wgrant) wrote :
Download full text (3.6 KiB)

On Fri, 2010-03-12 at 09:42 +0000, Björn Tillenius wrote:
> Review: Needs Information
> On Fri, Mar 12, 2010 at 07:20:53AM -0000, William Grant wrote:
> > William Grant has proposed merging lp:~wgrant/launchpad/bprdc into lp:launchpad with lp:~wgrant/launchpad/get-bpr-by-filename as a prerequisite.
> >
> > Requested reviews:
> > Björn Tillenius (bjornt): db
> > Stuart Bishop (stub): db
> > Canonical Launchpad Engineering (launchpad)
> >
> >
> > This branch adds the model for PPA binary download stats.
> > BinaryPackageReleaseDownloadCount is the class and DB table, with a new
> > Archive.updatePackageDownloadCount to create and update them. See bug
> > #139855 for discussion on the issue -- the only thing with which
> > everybody is agreed about collecting is counts for each binary package
> > release in each archive.
>
> I think this patch is ok, although I have a few questions (and a comment
> on the patch itself further down). I'll be on vacation next week, so I
> won't block a landing of the patch, waiting for the replies, as long as
> my comments are considered, but I'm marking it as needsinfo so that I
> can track the discussion more easily.

Thanks.

> vote needsinfo
>
> Can you explain shortly how BinaryPackageRelease relates to archive and
> series? As I understand it, this is only for PPAs, right, since we can't
> track the downloads of other archives? Would it make sense to make the
> name a bit more specific in that case? At the very least, comments.sql
> should explain this. Or will this be used by other archives than PPA
> ones?

A BinaryPackageRelease can be published in any number of archives and
distribution series (linked by BinaryPackagePublishingHistory). There
was talk a couple of months ago about a desire to use this for the
partner archive too -- I'm not sure if that's still on the cards, but
this schema works fine for that too, and the subsequent branches would
need only slight extension.

The primarily archive is more impossible, since it is heavily mirrored
across hundreds of hosts.

> I couldn't find where everybody agreed about collecting counts for each
> binary package release. Can you point me to the right place?
>
> What other things are we likely to want to track in the future, if any?

The last few comments all speak of counting binaries and indices, but no
concrete method of usefully counting index downloads has yet been
mentioned, and it's much much harder to get right (it involves lots of
guessing). Binary counting is easy and a good first step.

>
> > === added file 'database/schema/patch-2207-99-0.sql'
> > --- database/schema/patch-2207-99-0.sql 1970-01-01 00:00:00 +0000
> > +++ database/schema/patch-2207-99-0.sql 2010-03-12 07:20:45 +0000
> > @@ -0,0 +1,15 @@
> > +SET client_min_messages=ERROR;
> > +
> > +CREATE TABLE BinaryPackageReleaseDownloadCount (
> > + id serial PRIMARY KEY,
> > + archive integer NOT NULL REFERENCES Archive,
> > + binary_package_release integer NOT NULL REFERENCES BinaryPackageRelease,
> > + day date NOT NULL,
> > + country integer,
> > + count integer NOT NULL
> > +);
> > +
> > +ALTER TABLE BinaryPackageReleaseDownloadCount ADD CONSTRAINT binar...

Read more...

Revision history for this message
Stuart Bishop (stub) wrote :

The FOREIGN KEY reference to Country is correct. Thanks.

review: Approve (db)
Revision history for this message
Björn Tillenius (bjornt) :
review: Approve (db)
Revision history for this message
Henning Eggers (henninge) wrote :

All is well. Thank you. ;-)

review: Approve (code)
Revision history for this message
Julian Edwards (julian-edwards) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'database/schema/patch-2207-36-0.sql'
2--- database/schema/patch-2207-36-0.sql 1970-01-01 00:00:00 +0000
3+++ database/schema/patch-2207-36-0.sql 2010-03-15 10:41:18 +0000
4@@ -0,0 +1,15 @@
5+SET client_min_messages=ERROR;
6+
7+CREATE TABLE BinaryPackageReleaseDownloadCount (
8+ id serial PRIMARY KEY,
9+ archive integer NOT NULL REFERENCES Archive,
10+ binary_package_release integer NOT NULL REFERENCES BinaryPackageRelease,
11+ day date NOT NULL,
12+ country integer REFERENCES Country,
13+ count integer NOT NULL
14+);
15+
16+ALTER TABLE BinaryPackageReleaseDownloadCount ADD CONSTRAINT binarypackagereleasedownloadcount__archive__binary_package_release__day__country__key
17+ UNIQUE (archive, binary_package_release, day, country);
18+
19+INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 36, 0);
20
21=== modified file 'database/schema/security.cfg'
22--- database/schema/security.cfg 2010-03-15 06:23:18 +0000
23+++ database/schema/security.cfg 2010-03-15 10:41:18 +0000
24@@ -131,6 +131,7 @@
25 public.authtoken = SELECT, INSERT, UPDATE, DELETE
26 public.binaryandsourcepackagenameview = SELECT
27 public.binarypackagepublishinghistory = SELECT
28+public.binarypackagereleasedownloadcount= SELECT, INSERT, UPDATE
29 public.bountysubscription = SELECT, INSERT, UPDATE, DELETE
30 public.branchrevision = SELECT, INSERT, UPDATE, DELETE
31 public.branch = SELECT, INSERT, UPDATE, DELETE
32
33=== modified file 'lib/lp/soyuz/interfaces/archive.py'
34--- lib/lp/soyuz/interfaces/archive.py 2010-03-12 06:19:52 +0000
35+++ lib/lp/soyuz/interfaces/archive.py 2010-03-15 10:41:18 +0000
36@@ -647,6 +647,20 @@
37 archive.
38 """
39
40+ def updatePackageDownloadCount(bpr, day, country, count):
41+ """Update the daily download count for a given package.
42+
43+ :param bpr: The `IBinaryPackageRelease` to update the count for.
44+ :param day: The date to update the count for.
45+ :param country: The `ICountry` to update the count for.
46+ :param count: The new download count.
47+
48+ If there's no matching `IBinaryPackageReleaseDownloadCount` entry,
49+ we create one with the given count. Otherwise we just increase the
50+ count of the existing one by the given amount.
51+ """
52+
53+
54 class IArchiveView(IHasBuildRecords):
55 """Archive interface for operations restricted by view privilege."""
56
57
58=== modified file 'lib/lp/soyuz/interfaces/binarypackagerelease.py'
59--- lib/lp/soyuz/interfaces/binarypackagerelease.py 2009-06-25 04:06:00 +0000
60+++ lib/lp/soyuz/interfaces/binarypackagerelease.py 2010-03-15 10:41:18 +0000
61@@ -11,17 +11,18 @@
62 'BinaryPackageFileType',
63 'BinaryPackageFormat',
64 'IBinaryPackageRelease',
65+ 'IBinaryPackageReleaseDownloadCount',
66 'IBinaryPackageReleaseSet',
67 ]
68
69-from zope.schema import Bool, Int, Text, TextLine, Datetime
70+from lazr.enum import DBEnumeratedType, DBItem
71+from lazr.restful.fields import Reference
72+from zope.schema import Bool, Choice, Date, Int, Text, TextLine, Datetime
73 from zope.interface import Interface, Attribute
74
75 from canonical.launchpad import _
76-
77 from canonical.launchpad.validators.version import valid_debian_version
78-
79-from lazr.enum import DBEnumeratedType, DBItem
80+from lp.soyuz.interfaces.archive import IArchive
81
82
83 class IBinaryPackageRelease(Interface):
84@@ -79,6 +80,7 @@
85 argument remains untouched.
86 """
87
88+
89 class IBinaryPackageReleaseSet(Interface):
90 """A set of binary packages"""
91
92@@ -91,6 +93,23 @@
93 """Get an BinaryPackageRelease in a DistroSeries by its name"""
94
95
96+class IBinaryPackageReleaseDownloadCount(Interface):
97+ """Daily download count of a binary package release in an archive."""
98+
99+ archive = Reference(
100+ title=_('The archive'), schema=IArchive, required=True,
101+ readonly=True)
102+ binary_package_release = Reference(
103+ title=_('The binary package release'), schema=IBinaryPackageRelease,
104+ required=True, readonly=True)
105+ day = Date(
106+ title=_('The day of the downloads'), required=True, readonly=True)
107+ count = Int(
108+ title=_('The number of downloads'), required=True, readonly=False)
109+ country = Choice(
110+ title=_('Country'), required=False, vocabulary='CountryName')
111+
112+
113 class BinaryPackageFileType(DBEnumeratedType):
114 """Binary Package File Type
115
116
117=== modified file 'lib/lp/soyuz/model/archive.py'
118--- lib/lp/soyuz/model/archive.py 2010-03-12 06:19:52 +0000
119+++ lib/lp/soyuz/model/archive.py 2010-03-15 10:41:18 +0000
120@@ -39,6 +39,8 @@
121 from lp.soyuz.model.archivedependency import ArchiveDependency
122 from lp.soyuz.model.archiveauthtoken import ArchiveAuthToken
123 from lp.soyuz.model.archivesubscriber import ArchiveSubscriber
124+from lp.soyuz.model.binarypackagerelease import (
125+ BinaryPackageReleaseDownloadCount)
126 from lp.soyuz.model.build import Build
127 from lp.soyuz.model.distributionsourcepackagecache import (
128 DistributionSourcePackageCache)
129@@ -1327,6 +1329,19 @@
130 result_set.config(distinct=True).order_by(SourcePackageRelease.id)
131 return result_set
132
133+ def updatePackageDownloadCount(self, bpr, day, country, count):
134+ """See `IArchive`."""
135+ store = Store.of(self)
136+ entry = store.find(
137+ BinaryPackageReleaseDownloadCount, archive=self,
138+ binary_package_release=bpr, day=day, country=country).one()
139+ if entry is None:
140+ entry = BinaryPackageReleaseDownloadCount(
141+ archive=self, binary_package_release=bpr, day=day,
142+ country=country, count=count)
143+ else:
144+ entry.count += count
145+
146 def _setBuildStatuses(self, status):
147 """Update the pending Build Jobs' status for this archive."""
148
149
150=== modified file 'lib/lp/soyuz/model/binarypackagerelease.py'
151--- lib/lp/soyuz/model/binarypackagerelease.py 2009-06-25 04:06:00 +0000
152+++ lib/lp/soyuz/model/binarypackagerelease.py 2010-03-15 10:41:18 +0000
153@@ -4,18 +4,23 @@
154 # pylint: disable-msg=E0611,W0212
155
156 __metaclass__ = type
157-__all__ = ['BinaryPackageRelease', 'BinaryPackageReleaseSet']
158+__all__ = [
159+ 'BinaryPackageRelease',
160+ 'BinaryPackageReleaseDownloadCount',
161+ 'BinaryPackageReleaseSet'
162+ ]
163
164
165 from zope.interface import implements
166
167 from sqlobject import StringCol, ForeignKey, IntCol, SQLMultipleJoin, BoolCol
168+from storm.locals import Date, Int, Reference, Storm
169
170 from canonical.database.sqlbase import SQLBase, quote, sqlvalues, quote_like
171
172 from lp.soyuz.interfaces.binarypackagerelease import (
173 BinaryPackageFileType, BinaryPackageFormat, IBinaryPackageRelease,
174- IBinaryPackageReleaseSet)
175+ IBinaryPackageReleaseDownloadCount, IBinaryPackageReleaseSet)
176 from lp.soyuz.interfaces.publishing import (
177 PackagePublishingPriority, PackagePublishingStatus)
178 from canonical.database.enumcol import EnumCol
179@@ -199,3 +204,29 @@
180
181 return query, clauseTables
182
183+
184+class BinaryPackageReleaseDownloadCount(Storm):
185+ """See `IBinaryPackageReleaseDownloadCount`."""
186+
187+ implements(IBinaryPackageReleaseDownloadCount)
188+ __storm_table__ = 'BinaryPackageReleaseDownloadCount'
189+
190+ id = Int(primary=True)
191+ archive_id = Int(name='archive', allow_none=False)
192+ archive = Reference(archive_id, 'Archive.id')
193+ binary_package_release_id = Int(
194+ name='binary_package_release', allow_none=False)
195+ binary_package_release = Reference(
196+ binary_package_release_id, 'BinaryPackageRelease.id')
197+ day = Date(allow_none=False)
198+ country_id = Int(name='country', allow_none=True)
199+ country = Reference(country_id, 'Country.id')
200+ count = Int(allow_none=False)
201+
202+ def __init__(self, archive, binary_package_release, day, country, count):
203+ super(BinaryPackageReleaseDownloadCount, self).__init__()
204+ self.archive = archive
205+ self.binary_package_release = binary_package_release
206+ self.day = day
207+ self.country = country
208+ self.count = count
209
210=== modified file 'lib/lp/soyuz/tests/test_archive.py'
211--- lib/lp/soyuz/tests/test_archive.py 2010-03-12 06:19:52 +0000
212+++ lib/lp/soyuz/tests/test_archive.py 2010-03-15 10:41:18 +0000
213@@ -3,7 +3,7 @@
214
215 """Test Archive features."""
216
217-from datetime import datetime, timedelta
218+from datetime import date, datetime, timedelta
219 import pytz
220 import unittest
221
222@@ -19,6 +19,7 @@
223 from lp.registry.interfaces.distribution import IDistributionSet
224 from lp.registry.interfaces.person import IPersonSet
225 from lp.services.job.interfaces.job import JobStatus
226+from lp.services.worlddata.interfaces.country import ICountrySet
227 from lp.soyuz.interfaces.archive import (
228 IArchiveSet, ArchivePurpose, CannotSwitchPrivacy)
229 from lp.soyuz.interfaces.archivearch import IArchiveArchSet
230@@ -26,6 +27,8 @@
231 from lp.soyuz.interfaces.processor import IProcessorFamilySet
232 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
233 from lp.soyuz.model.build import Build
234+from lp.soyuz.model.binarypackagerelease import (
235+ BinaryPackageReleaseDownloadCount)
236 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
237 from lp.testing import TestCaseWithFactory
238
239@@ -562,6 +565,105 @@
240 self.assertEqual('0.5.11~ppa1', pubs[0].source_package_version)
241
242
243+class TestUpdatePackageDownloadCount(TestCaseWithFactory):
244+ """Ensure that updatePackageDownloadCount works as expected."""
245+
246+ layer = LaunchpadZopelessLayer
247+
248+ def setUp(self):
249+ super(TestUpdatePackageDownloadCount, self).setUp()
250+ self.publisher = SoyuzTestPublisher()
251+ self.publisher.prepareBreezyAutotest()
252+
253+ self.store = getUtility(IStoreSelector).get(
254+ MAIN_STORE, DEFAULT_FLAVOR)
255+
256+ self.archive = self.factory.makeArchive()
257+ self.bpr_1 = self.publisher.getPubBinaries(
258+ archive=self.archive)[0].binarypackagerelease
259+ self.bpr_2 = self.publisher.getPubBinaries(
260+ archive=self.archive)[0].binarypackagerelease
261+
262+ country_set = getUtility(ICountrySet)
263+ self.australia = country_set['AU']
264+ self.new_zealand = country_set['NZ']
265+
266+ def assertCount(self, count, archive, bpr, day, country):
267+ self.assertEqual(count, self.store.find(
268+ BinaryPackageReleaseDownloadCount,
269+ archive=archive, binary_package_release=bpr,
270+ day=day, country=country).one().count)
271+
272+ def test_creates_new_entry(self):
273+ # The first update for a particular archive, package, day and
274+ # country will create a new BinaryPackageReleaseDownloadCount
275+ # entry.
276+ day = date(2010, 2, 20)
277+ self.assertIs(None, self.store.find(
278+ BinaryPackageReleaseDownloadCount,
279+ archive=self.archive, binary_package_release=self.bpr_1,
280+ day=day, country=self.australia).one())
281+ self.archive.updatePackageDownloadCount(
282+ self.bpr_1, day, self.australia, 10)
283+ self.assertCount(10, self.archive, self.bpr_1, day, self.australia)
284+
285+ def test_reuses_existing_entry(self):
286+ # A second update will simply add to the count on the existing
287+ # BPRDC.
288+ day = date(2010, 2, 20)
289+ self.archive.updatePackageDownloadCount(
290+ self.bpr_1, day, self.australia, 10)
291+ self.archive.updatePackageDownloadCount(
292+ self.bpr_1, day, self.australia, 3)
293+ self.assertCount(13, self.archive, self.bpr_1, day, self.australia)
294+
295+ def test_differentiates_between_countries(self):
296+ # A different country will cause a new entry to be created.
297+ day = date(2010, 2, 20)
298+ self.archive.updatePackageDownloadCount(
299+ self.bpr_1, day, self.australia, 10)
300+ self.archive.updatePackageDownloadCount(
301+ self.bpr_1, day, self.new_zealand, 3)
302+
303+ self.assertCount(10, self.archive, self.bpr_1, day, self.australia)
304+ self.assertCount(3, self.archive, self.bpr_1, day, self.new_zealand)
305+
306+ def test_country_can_be_none(self):
307+ # The country can be None, indicating that it is unknown.
308+ day = date(2010, 2, 20)
309+ self.archive.updatePackageDownloadCount(
310+ self.bpr_1, day, self.australia, 10)
311+ self.archive.updatePackageDownloadCount(
312+ self.bpr_1, day, None, 3)
313+
314+ self.assertCount(10, self.archive, self.bpr_1, day, self.australia)
315+ self.assertCount(3, self.archive, self.bpr_1, day, None)
316+
317+ def test_differentiates_between_days(self):
318+ # A different date will also cause a new entry to be created.
319+ day = date(2010, 2, 20)
320+ another_day = date(2010, 2, 21)
321+ self.archive.updatePackageDownloadCount(
322+ self.bpr_1, day, self.australia, 10)
323+ self.archive.updatePackageDownloadCount(
324+ self.bpr_1, another_day, self.australia, 3)
325+
326+ self.assertCount(10, self.archive, self.bpr_1, day, self.australia)
327+ self.assertCount(
328+ 3, self.archive, self.bpr_1, another_day, self.australia)
329+
330+ def test_differentiates_between_bprs(self):
331+ # And even a different package will create a new entry.
332+ day = date(2010, 2, 20)
333+ self.archive.updatePackageDownloadCount(
334+ self.bpr_1, day, self.australia, 10)
335+ self.archive.updatePackageDownloadCount(
336+ self.bpr_2, day, self.australia, 3)
337+
338+ self.assertCount(10, self.archive, self.bpr_1, day, self.australia)
339+ self.assertCount(3, self.archive, self.bpr_2, day, self.australia)
340+
341+
342 class TestARMBuildsAllowed(TestCaseWithFactory):
343 """Ensure that ARM builds can be allowed and disallowed correctly."""
344

Subscribers

People subscribed via source and target branches

to status/vote changes: