-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix(kv): add KV migration to repair DBRP mappings broken by schema change #20168
Conversation
53bf493
to
592d0fd
Compare
Marking as draft while I convert the logic to a KV migration (thanks for the idea @stuartcarnie!) |
1194304
to
2830bf3
Compare
2830bf3
to
b6a16c0
Compare
// repair DBRP owner and bucket IDs | ||
Migration0013_RepairDBRPOwnerAndBucketIDs, | ||
// reindex DBRPs | ||
Migration0014_ReindexDBRPs, |
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.
The reindex here is required for the Find
methods in the DBRP service to locate repaired DBRPs. Reordering the migrations so my new Migration0013
runs before the existing Migration0012
also works, but that felt like a potentially-dangerous idea.
"github.com/influxdata/influxdb/v2/kv" | ||
) | ||
|
||
var Migration0013_RepairDBRPOwnerAndBucketIDs = UpOnlyMigration( |
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.
The logic & structure of this file is largely copied from the 0012 migration.
To test this change, I:
|
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.
Awesome. Cheers @danxmoran !
Closes #20167
I tested the new command locally, happy to refactor and set up automated tests if the reviewers would like.