chore: Use an optimized schema for Cassandra #513
Conversation
Also added the mention of Scylla on the README fix: golang-migrate#455
Pull Request Test Coverage Report for Build 153
💛 - Coveralls |
dhui
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Honestly, this would be a lot simpler if the db drivers used migrate to manage the schema versions... But that's a lot more work. e.g. figure out how to store the schema versions for the schema versions...
This approach is fine for now but isn't sustainable if the schema versions table continues to evolve.
| * [Redshift](database/redshift) | ||
| * [Ql](database/ql) | ||
| * [Cassandra](database/cassandra) | ||
| * [Cassandra/ Scylla](database/cassandra) |
There was a problem hiding this comment.
Drop the mention of scylla unless there are tests against it.
There was a problem hiding this comment.
I thought this would be fine since MariaDB is also mentioned despite not being tested as of now; I'll add tests
| return &database.Error{OrigErr: err, Query: []byte(query)} | ||
| } | ||
| } else { | ||
| query := `DELETE FROM "` + c.config.MigrationsTable + `" WHERE dummy = 1` |
There was a problem hiding this comment.
This is to reproduce the original behavior of the TRUNCATE that were present before, which would have left the database empty if the condition wasn't met
| query := `SELECT version, dirty FROM "` + c.config.MigrationsTable + `" LIMIT 1` | ||
| if err = c.session.Query(query).Scan(&version, &dirty); err != nil { | ||
| if err == gocql.ErrNotFound { | ||
| skip = true |
There was a problem hiding this comment.
Upgrades should only be performed when we detect that an upgrade is needed.
Given that, we should error on attempted schema upgrade failures. e.g. failing to fetch the previous applied migration version & status when upgrading the cassandra schema table is a stopping error condition
Also, the previous version and status should be logged so subsequent failures can be manually recovered.
There was a problem hiding this comment.
failing to fetch the previous applied migration version
Given the previous implementation, the table may have been left in an empty state since the table is always emptied when SetVersion is called, but only filled back if a condition is met (and if it is not, no error is returned). This is why I don't error if it can't find previous version and instead just skip adding back the data
Also, the previous version and status should be logged so subsequent failures can be manually recovered.
That's a valid point, I'll add some logging
| }) | ||
| } | ||
|
|
||
| func TestSchemaVersionV2Migration(t *testing.T) { |
There was a problem hiding this comment.
Thanks for adding a test!
Also add a test for the case where upgradeVersionTable() is a noop.
| } | ||
| if _, _, err = c.Version(); err != nil { | ||
| var count int8 | ||
| err = c.session.Query(`SELECT COUNT(*) FROM system_schema.columns WHERE keyspace_name = '` + c.config.KeyspaceName + `' AND table_name = '` + c.config.MigrationsTable + `' AND column_name = 'dummy'`).Scan(&count) |
There was a problem hiding this comment.
Why not move this into createVersionTable() and make createVersionTable() call upgradeVersionTable()?
There was a problem hiding this comment.
I've put it here so in normal circumstances, this query is only performed if there is an error (which might indicate the table needs an update). Moving this in createVersionTable would mean there'd always be 2 calls done instead of 1
I think there'd still be a catch in this case, as we are changing the primary key which requires dropping the table and building it back, and hold previous data in memory so we can insert it back, so it would most likely need specific code at some point anyway. |
|
@dhui Hi, would you merge this PR if I will prepare scylladb tests as you asked above? |
Uses the schema proposed in #455, with a built-in upgrade script which makes the change transparent (and avoiding the breaking change). I added a test case to ensure the upgrade code does what it's supposed to do.
fix: #455
I also added the mention of Scylla on the README, since this tool has been working like a charm for it on my end 🎉 (not really surprising since it aims to be a drop-in Cassandra replacement)