Code review comment for lp:~lifeless/launchpad/private-librarian

The idea seems good.

I don't see why the table should exist in the session database. It should exist in the main database. Putting it in the session database just doubles the number of database connections the Librarian and garbage collector need to maintain, and makes things a maintenance pain as that database has to have schema changes and database permissions applied manually to the primary and fail over databases. This table will get comparatively little traffic, so scalability issues are a non-issue.

I believe the blessed import location for the Storm class is from storm.locals.

Should we be storing the expiry timestamp? I'm wondering if the length of time a token is valid for should be a function of the file size, or if it will be different for different parts of Launchpad.

The index on (url, token) is redundant, as it is created implicitly by the primary key constraint.

Do we actually need to store url and token separately, or just a single url column that includes the query string?

Do we want to store the url, or just the path segment? Storing just the path segment would avoid ambiguities between host and host:port, and might future proof things a bit if our SSL front ends get distributed around the world on different domain names.

Are their any magic headers the Librarian can emit so if a launchpadlibrarian.net URL is bookmarked, the original launchpad.net URL is bookmarked instead? Not terribly important, but if we can do this we will need to store the source URL too as the Librarian has no way of reconstructing it.