Code review comment for lp:~stub/launchpad/replication

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/