search

cloudspannerecosystem / wrench

wrench - Schema management tool for Cloud Spanner -
MIT License
239 stars 47 forks source link

refactoring suggestion - migration table name is hardcoded in both pkg/spanner and cmd/ #50

Open fcostin opened 2 years ago

fcostin commented 2 years ago

while reviewing the code I noticed a minor issue with internal code structure, does not impact functionality of wrench tool:

Currently, it is not clear which layer of the code is responsible for choosing the default migration table name:

  1. the public API exposed by pkg/spanner Client has a tableName parameter on most operations that interact with the migration table, with the exception of TruncateAllTables, which hardcodes the table name instead of letting the caller control it: https://github.com/cloudspannerecosystem/wrench/blob/9f346f26805724d5361d96ea8f3fb6b9cacfeb48/pkg/spanner/client.go#L134
  2. cmd/migrate.go also hardcodes the migration table name: https://github.com/cloudspannerecosystem/wrench/blob/9f346f26805724d5361d96ea8f3fb6b9cacfeb48/cmd/migrate.go#L36

The code structure could be slightly cleaner if only one layer of the code was responsible for choosing the migration table name.

I can think of a few options for refactoring to clarify this, but some have potential downsides of changing the API of exported methods of pkg/spanner Client . If it is important to not make breaking changes to pkg/spanner Client API (if users are using that as a library from their projects without using wrench command line tool) then it might be best not to do that.

Suggested options:

option description code structure breaking change to wrench tool? breaking change to exported wrench pkg/spanner Client API?
a current structure slightly unclear NA, no change NA, no change
b add tableName to TruncateAllTables slightly clearer no change breaking change!
c add TruncateAllTablesV2 with tableName, deprecate TruncateAllTables slightly clearer no change no change
d remove tableName from pkg/spanner Client, allow custom migration table name to be set using pkg/spanner Config slightly clearer no change breaking change!
fcostin commented 2 years ago

I have raised a draft PR to explore what "option d" suggested above could look like: https://github.com/cloudspannerecosystem/wrench/pull/51

that proposal has downside of being a breaking change to pkg/spanner Client API.

asetty commented 1 year ago

Kind of on topic here, I have a use case where I'd like to manage migration separately for multiple sets of tables colocated in the same DB. We could add a migration table flag to the CLI tool to support this. I started making changes on a branch in my fork, but will need to add some testing probably.

Also some error checking for maybe:

  1. migrations table is valid ( in case of typos)
  2. all tables modified in migration do not conflict with other migration table's tables

One idea I have is creating a owners table that maps migration table to DB table in order to check this. I may create a separate issue, but would be interested if someone replied here. It would be nice to merge your PR https://github.com/cloudspannerecosystem/wrench/pull/72 first