Code review comment for lp:~wgrant/launchpad/bprdc

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 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?

Good point. Fixed.

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

Another good point. I've added a test.