Merge lp:~stub/launchpad/replication into lp:launchpad

Fix Bug #461800

new-slave.py is a script used on production to build a new slave database. It automates a non-trivial process. This process became even less trivial when we moved the authdb replication set master to a seperate database than the lpmain replication set master. One of the first steps this script needs to do is duplicate the schema. Unfortunately, with the 1.2 series of Slony-I, backups from slaves replication sets are incomplete (missing triggers etc.). So now we need to build the schema using two databases rather than the single database used previously (the master for both replication sets).

merge-conditional, so that you can get ahead with that when you get up ... ;-)

Thank you for improving this. I just have two improvements that I'd like to have in there.

> + # Get the connection string for masters.
> + lpmain_connection_string = get_master_connection_string(
> + source_connection, parser, 1) or source_connection_string
> + authdb_connection_string = get_master_connection_string(
> + source_connection, parser, 2) or source_connection_string

Please define constants for "1" and "2", e.g. "LPMAIN_SET_ID = 1".

> + # Duplicate the authdb schema.
> + log.info("Duplicating authdb schema from '%s' to '%s'" % (
> + authdb_connection_string, target_connection_string))
> + table_args = ["--table=%s" % table for table in _auth_store_tables]
> + # We need to restore the two cross-replication-set views that where
> + # dropped as a side effect of dropping the auth store tables.
> + table_args.append("--table=ValidPersonCache")
> + table_args.append("--table=ValidPersonOrTeamCache")

Please list these two tables in a list, close to where you define _auth_store_tables. This keeps this information from being scattered throughout the code. You can then create table_args directly from the two lists, like this:

table_args = ["--table=%s" % table for table in _auth_store_tables + whatever_you_call_them_tables]

BTW, since _auth_store_tables is imported from somewhere else, the leading _ is missleading, isn't it?

Cheers,
Henning

Assuming this is all well tested, let's get it in devel while we can!

On Mon, Nov 2, 2009 at 11:07 PM, Henning Eggers
<email address hidden> wrote:
> Review: Approve code
> merge-conditional, so that you can get ahead with that when you get up ... ;-)
>
> Thank you for improving this. I just have two improvements that I'd like to have in there.
>
>> + # Get the connection string for masters.
>> + lpmain_connection_string = get_master_connection_string(
>> + source_connection, parser, 1) or source_connection_string
>> + authdb_connection_string = get_master_connection_string(
>> + source_connection, parser, 2) or source_connection_string
>
> Please define constants for "1" and "2", e.g. "LPMAIN_SET_ID = 1".

Done.

>> + # Duplicate the authdb schema.
>> + log.info("Duplicating authdb schema from '%s' to '%s'" % (
>> + authdb_connection_string, target_connection_string))
>> + table_args = ["--table=%s" % table for table in _auth_store_tables]
>> + # We need to restore the two cross-replication-set views that where
>> + # dropped as a side effect of dropping the auth store tables.
>> + table_args.append("--table=ValidPersonCache")
>> + table_args.append("--table=ValidPersonOrTeamCache")
>
> Please list these two tables in a list, close to where you define _auth_store_tables. This keeps this information from being scattered throughout the code. You can then create table_args directly from the two lists, like this:
>
> table_args = ["--table=%s" % table for table in _auth_store_tables + whatever_you_call_them_tables]

I'm leaving this for next time. These two views are an atavism and
don't belong in canonical.launchpad.webapp.adapter. Hopefully next
time they won't be there at all as we split authentication and
launchpad apart further.

> BTW, since _auth_store_tables is imported from somewhere else, the leading _ is missleading, isn't it?

It is. There needs to be a better way of the database adapter
determining which Store should be used for a table than having this
list hardcoded. It probably should be an atttribute on the database
class. The _ is still appropriate as we don't want this as part of the
public API. Hopefully these chages to new-slave.py are just a
temporary hack and we can simplify things in the near future.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/