Skip to content
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

feat(kv): support for migrations #17145

Merged
merged 7 commits into from
Mar 18, 2020
Merged

feat(kv): support for migrations #17145

merged 7 commits into from
Mar 18, 2020

Conversation

GeorgeMac
Copy link
Contributor

@GeorgeMac GeorgeMac commented Mar 9, 2020

This change introduces a new kv.Migrator and associated migrations types.
This change sets a new precedent for how we introduce new resource types and indexes into the kv package within InfluxDB.

The new Migrator type adds support for managing these ongoing types of change as migrations. A migration is effectively a change to kv schema (defined as an Up(...) function operation) and a definition of how to revert the operation (defined as a Down(...) function operation).

The kinds of "schema" changes expected to be managed by these types (in the near-team) are KV buckets and indexes.

By default, inmem and bolt kv implementations should continue to operate as they always have. On initialization of the KV service, when backed by one of these KV stores implementations the migrations are all applied automatically.

Outstanding:

  • Get feat(kv): index utility type #16910 shipped
  • Consider either moving the introduction of the new indexes into own PR.
  • move tests into testing package and invoke for both inmem and bolt.

Checklist:

  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Well-formatted commit messages
  • Rebased/mergeable
  • Tests pass
  • http/swagger.yml updated (if modified Go structs or API)
  • Documentation updated or issue created (provide link to issue/pr)
  • Signed CLA (if not already signed)

@GeorgeMac GeorgeMac requested review from a team and stuartcarnie and removed request for a team and stuartcarnie March 10, 2020 15:21
@GeorgeMac GeorgeMac changed the base branch from gm/indexer to master March 11, 2020 14:59
@GeorgeMac GeorgeMac marked this pull request as ready for review March 11, 2020 15:54
@GeorgeMac GeorgeMac requested review from a team and gianarb and removed request for a team March 11, 2020 15:54

// ErrMigrationSpecNotFound is returned when a migration specification is missing
// for an already applied migration.
ErrMigrationSpecNotFound = errors.New("migration specification not found")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

be good to make this an inflxudb.Error type with the necessary code/msg

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of error shouldn't really ever reach a user

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It happens on initialize or through manual invocation via a cli.

@GeorgeMac GeorgeMac force-pushed the gm/migrations branch 2 times, most recently from 3264284 to 95d334f Compare March 13, 2020 13:50
@GeorgeMac GeorgeMac requested review from a team, aanthony1243 and brettbuddin and removed request for gianarb, a team and aanthony1243 March 16, 2020 16:05
Copy link
Contributor

@brettbuddin brettbuddin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor questions. Take them or leave them.

kv/index.go Show resolved Hide resolved
kv/migration.go Outdated Show resolved Hide resolved
Copy link
Contributor

@affo affo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GeorgeMac this is huge.

I really like it, thanks for doing this.

The framework is pretty elastic and enables all sorts of migrations given that you have access to the underlying kv in Up().

In this scenario migrations would need to be applied via CD, right?

Up() and Down() are in the codebase, and not part of the Migration object itself.
So, to enable a migration, you'll need to code it and deploy it, or am I missing something?

We could take this offline and think about how we could expose this as an HTTP service, what do you think? This way one could run migrations independently from code base changes.

@GeorgeMac GeorgeMac merged commit 1d400a4 into master Mar 18, 2020
@GeorgeMac GeorgeMac deleted the gm/migrations branch March 18, 2020 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants