-
Notifications
You must be signed in to change notification settings - Fork 121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Await schema agreement during migrations #133
Await schema agreement during migrations #133
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, the upstream change we needed was merged and it's working. Any other gripes or thoughts on getting this merged? There's only one comment as I'd like to not make this configurable but it's your choice. It awaits schema agreement before executing each statement so if the cluster is in a bad state it won't even apply a single migration. It also does a final await before calling it done.
This needs to be configurable with the following options
There is no point in applying it after each statement (sent to the same host) or before anything. |
Hmmm, but I don't want to apply even one statement if the cluster is in a bad state (nodes unavailable). Also, I'm usually putting both dropping a materialized view and recreating it into the same migration and these statements should only be applied one after another with the schema agreed cluster-wide. I already have this version of gocqlx in production right now. The same host doesn't really matter if all nodes agree on the schema or am I missing something? With the current variable at the top I only need to add it to the final call after all the migrations have successfully been applied. WDYT? |
The final call should be mandatory. We can have the enum extended to
|
If I get you to agree to change it to this:
Then I would agree. I'm reiterating: I don't want to apply a single statement if the cluster is in an unhealthy state or doesn't have schema agreement. With your proposal that wouldn't be possible or require me to add another one before starting the migrations (something I don't have to do if I change both modes to check before instead of after). |
Optional before (file or statement) plus mandatory after full migration is perfect. |
Gave it a go with an enum. I hope you like it. :) |
migrate/migrate.go
Outdated
awaitSchemaAgreementAfterMigrations | ||
) | ||
|
||
func (as awaitSchemaAgreement) Check(ctx context.Context, session *gocql.Session, stage awaitSchemaAgreement) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my view it would be better to separate the fileterig (SchouldAwait) logic from the session.AwaitSchemaAgreement
call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry about that – done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh damn, didn't see your second comment there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean I should change. Can you elaborate? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take out session.AwaitSchemaAgreement(ctx)
from this function and rename to SchouldAwait
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All done. Take another look? I liked the previous version better because it saved having to use 2 nested if
s but it's your call. :)
Depending on what is set, schema agreement is checked before each file or before each statement. Awaiting schema agreement after every migration has run is always done.
Running this one last time in our infra inside a GitLab CI job with |
Works as advertised. 🎉 Migrations take a bit longer than before (which is expected) but I'm now confident that I won't run into any problems with migrations ever again (apart from them failing because of node issues). |
Thanks. |
This uses
(*gocql.Session).AwaitSchemaAgreement
before applying each CQL statement in a migration (see apache/cassandra-gocql-driver#1301).It's marked as a draft for discussion as well as because it's still waiting on apache/cassandra-gocql-driver#1395 to be merged and is currently using our fork of gocql.