sqlmigrations: don't run baked-in migrations#41914
Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom Oct 31, 2019
Merged
sqlmigrations: don't run baked-in migrations#41914craig[bot] merged 2 commits intocockroachdb:masterfrom
craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
Member
|
This change is |
tbg
approved these changes
Oct 25, 2019
Member
tbg
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 6 of 6 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @dt)
pkg/sqlmigrations/migrations.go, line 271 at r2 (raw file):
// range is created at bootstrap time. Otherwise, we'd have the split queue // asynchronously creating some ranges which is annoying for tests. includedInBootstrap roachpb.Version
Explain what I set this to when adding a new migration.
Release note: None
97d426f to
bf40029
Compare
andreimatei
commented
Oct 31, 2019
Contributor
Author
andreimatei
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sqlmigrations/migrations.go, line 271 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Explain what I set this to when adding a new migration.
done
Before this patch, the migration runner had a mechanism by which it attempted to not run migrations that were included in the cluster bootstrap schema. That mechanism was weak. A migration could be marked as includedInBootstrap, and then the migration runner would skip it if the current node had just performed a bootstrap. However, the respective migration would still be run (aimlessly) by a 2nd node joining the cluster, or even by the same node that just skipped them, after a restart. This patch improves the mechanism such that migrations included in the bootstrap image are never run if the cluster's bootstrap version is high enough. It does so by allowing one to mark migrations with the version at which they've been introduced, and the runner will skip them if the cluster's bootstrap version is >=. This means that migrations that want to be marked in this way also need to introduce a cluster version. The benefit of this change, besides faster node startup time, removal of confusing entries from the system.event_log table and more determinism (because now some migrations are never run instead of not run on single node clusters but run on multi-node clusters), is that logic tests that hardcode expected cluster events are more easily written. Before this patch, those tests had to explicitly ignore some events that are generated by migrations (because, again, depending on the configuration, the migration would sometimes run and sometimes not). It's now the second time this happens to a change that I personally want to make, and it's very surprising and scary when it happens to you. You can see the edit in the event_log logic test for a case that I'm not correcting (there's probably others in the same file but I haven't looked). Release note: None.
bf40029 to
1dd1277
Compare
andreimatei
commented
Oct 31, 2019
Contributor
Author
andreimatei
left a comment
There was a problem hiding this comment.
bors r+
Reviewable status:
craig bot
pushed a commit
that referenced
this pull request
Oct 31, 2019
41914: sqlmigrations: don't run baked-in migrations r=andreimatei a=andreimatei Before this patch, the migration runner had a mechanism by which it attempted to not run migrations that were included in the cluster bootstrap schema. That mechanism was weak. A migration could be marked as includedInBootstrap, and then the migration runner would skip it if the current node had just performed a bootstrap. However, the respective migration would still be run (aimlessly) by a 2nd node joining the cluster, or even by the same node that just skipped them, after a restart. This patch improves the mechanism such that migrations included in the bootstrap image are never run if the cluster's bootstrap version is high enough. It does so by allowing one to mark migrations with the version at which they've been introduced, and the runner will skip them if the cluster's bootstrap version is >=. This means that migrations that want to be marked in this way also need to introduce a cluster version. The benefit of this change, besides faster node startup time, removal of confusing entries from the system.event_log table and more determinism (because now some migrations are never run instead of not run on single node clusters but run on multi-node clusters), is that logic tests that hardcode expected cluster events are more easily written. Before this patch, those tests had to explicitly ignore some events that are generated by migrations (because, again, depending on the configuration, the migration would sometimes run and sometimes not). It's now the second time this happens to a change that I personally want to make, and it's very surprising and scary when it happens to you. You can see the edit in the event_log logic test for a case that I'm not correcting (there's probably others in the same file but I haven't looked). Release note: None. 41991: storage/engine: move MVCCIncrementalIterator from storageccl to engine r=petermattis a=hueypark This is prerequisites for [storage/engine: add Pebble support to ExportToSst](#41739). Recovered some tests from [engine: Add ExportToSst method with all logic moved to C++](75f1314). See #41945 Release note: None 42066: sql: SHOW ZONE CONFIGURATION only if user has permissions r=otan a=otan Previously, SHOW ZONE CONFIGURATION did not respect permissions on tables. Patch that up by checking permissions when using this command. Resolves the command in #40917. Has merge conflicts with #42046; I will fix them up with whatever lands first. Release note (bug fix): fix bug where zones would display in `SHOW ZONE CONFIGURATION` even though the user has no permission to view that table Co-authored-by: Andrei Matei <andrei@cockroachlabs.com> Co-authored-by: Jaewan Park <jaewan.huey.park@gmail.com> Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Contributor
Build succeeded |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before this patch, the migration runner had a mechanism by which it
attempted to not run migrations that were included in the cluster
bootstrap schema. That mechanism was weak. A migration could be marked
as includedInBootstrap, and then the migration runner would skip it if
the current node had just performed a bootstrap. However, the respective
migration would still be run (aimlessly) by a 2nd node joining the
cluster, or even by the same node that just skipped them, after a restart.
This patch improves the mechanism such that migrations included in the
bootstrap image are never run if the cluster's bootstrap version is high
enough. It does so by allowing one to mark migrations with the version
at which they've been introduced, and the runner will skip them if the
cluster's bootstrap version is >=. This means that migrations that want
to be marked in this way also need to introduce a cluster version.
The benefit of this change, besides faster node startup time, removal of
confusing entries from the system.event_log table and more
determinism (because now some migrations are never run instead of not
run on single node clusters but run on multi-node clusters), is that
logic tests that hardcode expected cluster events are more easily
written. Before this patch, those tests had to explicitly ignore some
events that are generated by migrations (because, again, depending on
the configuration, the migration would sometimes run and sometimes not).
It's now the second time this happens to a change that I personally want
to make, and it's very surprising and scary when it happens to you. You
can see the edit in the event_log logic test for a case that I'm not
correcting (there's probably others in the same file but I haven't
looked).
Release note: None.