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

migrations: populate initial version setting #17694

Merged
merged 2 commits into from
Aug 16, 2017

Conversation

tbg
Copy link
Member

@tbg tbg commented Aug 16, 2017

Add a bootstrap version which is populated when a cluster running >1.0 is
bootstrapped. Use it to populate the settings table via a sql migration. At the
same time, prevent SET CLUSTER SETTING version = 'x' from working until that
migration has run.

This solves one remaining headache for version migrations by giving it
authoritative information on the currently running cluster version during
upgrades.

Fixes #17389.

@tbg tbg requested review from a team August 16, 2017 18:27
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg tbg force-pushed the bootstrap-version branch 2 times, most recently from d63ccb8 to 35854f9 Compare August 16, 2017 18:55
This uses the previously added bootstrap version which is populated when a
cluster running >1.0 is bootstrapped to populate the settings table. At the
same time, prevent `SET CLUSTER SETTING version = 'x'` from working until
that migration has run.

This solves one remaining headache for [version migrations][1] by giving it
authoritative information on the currently running cluster version during
upgrades.

Fixes cockroachdb#17389.

[1]: cockroachdb#16977
@tbg tbg force-pushed the bootstrap-version branch from 35854f9 to eddc62c Compare August 16, 2017 19:30
@bdarnell
Copy link
Contributor

:lgtm:


Reviewed 8 of 8 files at r1, 8 of 8 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


pkg/migration/sqlmigrations/migrations.go, line 416 at r2 (raw file):

func populateVersionSetting(ctx context.Context, r runner) error {
	var v roachpb.Version
	if err := r.db.Txn(ctx, func(ctx context.Context, txn *client.Txn) error {

Can this be r.db.Get instead of using an explicit Txn?


Comments from Reviewable

@tbg
Copy link
Member Author

tbg commented Aug 16, 2017

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


pkg/migration/sqlmigrations/migrations.go, line 416 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Can this be r.db.Get instead of using an explicit Txn?

Yes, but I'd have to plumb more stuff onto runner, and so I decided against it. I can add a comment and update the runner, whichever you prefer.


Comments from Reviewable

@bdarnell
Copy link
Contributor

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


pkg/migration/sqlmigrations/migrations.go, line 416 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Yes, but I'd have to plumb more stuff onto runner, and so I decided against it. I can add a comment and update the runner, whichever you prefer.

Leaving it is fine; I didn't realize this wasn't a full client.DB


Comments from Reviewable

@tbg
Copy link
Member Author

tbg commented Aug 16, 2017

TFTR, Ben!

@tbg tbg merged commit ec307cd into cockroachdb:master Aug 16, 2017
@tbg tbg deleted the bootstrap-version branch May 8, 2018 15:04
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.

3 participants