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

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