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

Add cluster version as feature gate for block properties #76278

Merged
merged 2 commits into from
Feb 15, 2022

Conversation

nicktrav
Copy link
Collaborator

@nicktrav nicktrav commented Feb 9, 2022

Two commits here - the first adds a new cluster version, the second makes use of the cluster version as a feature gate (and update various call sites all over the place).


pkg/clusterversion: add new version as feature gate for block properties
Prior to this change, the block properties SSTable-level feature was
enabled in a single cluster version. This introduced a subtle race in
that for the period in which each node is being updated to
PebbleFormatBlockPropertyCollector, there is a brief period where not
all nodes are at the same cluster version, and thus store versions. If
nodes at the newer version write SSTables that make use of block
properties, and these tables are consumed by nodes that are yet to be
updated, the older nodes could panic when attempting to read the tables
with a format they do not yet understand.

While this race is academic, given that a) there are now subsequent
cluster versions that act as barriers during the migration, and b) block
properties were disabled in 1a8fb57, this patch addresses the race by
adding a second cluster version.
EnablePebbleFormatVersionBlockProperties acts as a barrier and a
feature gate. A guarantee of the migration framework is that any node at
this newer version is part of a cluster that has already run the
necessary migrations for the older version, and thus ratcheted the
format major version in the store, and thus enabled the block properties
feature, across all nodes.

Add additional documentation in pebble.go that details how to make use
of the two-version pattern for future table-level version changes.


pkg/storage: make MakeIngestionWriterOptions version aware
With the introduction of block properties, and soon, range keys, which
introduce backward-incompatible changes at the SSTable level, all nodes
in a cluster must all have a sufficient store version in order to avoid
runtime incompatibilities.

Update storage.MakeIngestionWriterOptions to add a context.Context
and cluster.Settings as parameters, which allows for determining
whether a given cluster version is active (via
(clusterversion.Handle).IsActive()). This allows gating the enabling /
disabling of block properties (and soon, range keys), on all nodes being
at a sufficient cluster version.

Update various call-sites to pass in the context.Context and
cluster.Settings.


@nicktrav nicktrav requested review from a team as code owners February 9, 2022 05:11
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@nicktrav nicktrav requested review from jbowens and dt February 9, 2022 05:11
// TODO(travers): Add metamorphic test support for different versions, which
// will give us better coverage across multiple format major versions and
// table versions.
m.st = cluster.MakeTestingClusterSettings()
Copy link
Member

@dt dt Feb 9, 2022

Choose a reason for hiding this comment

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

I get really nervous when I see a MakeTestingClusterSettings in a non-_test.go file. I gather from looking around a little more that this is in fact a test file so I guess I shouldn't be. Part of me still wishes this was a parameter used to create the metaTestRunner so the make call was in a main or _test package all the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair point! It was easy to move it to a _test file.

I'm thinking that addressing the TODO can probably remove the need for MakeTestingClusterSettings entirely, and instead set up a cluster version in a "more real" way.

@nicktrav nicktrav force-pushed the nickt.sst-writer-engine branch from d44b1e7 to d87d8f1 Compare February 9, 2022 15:08
@nicktrav
Copy link
Collaborator Author

nicktrav commented Feb 10, 2022

Linking to #76327, on which this one is blocked.

Prior to this change, the block properties SSTable-level feature was
enabled in a single cluster version. This introduced a subtle race in
that for the period in which each node is being updated to
`PebbleFormatBlockPropertyCollector`, there is a brief period where not
all nodes are at the same cluster version, and thus store versions. If
nodes at the newer version write SSTables that make use of block
properties, and these tables are consumed by nodes that are yet to be
updated, the older nodes could panic when attempting to read the tables
with a format they do not yet understand.

While this race is academic, given that a) there are now subsequent
cluster versions that act as barriers during the migration, and b) block
properties were disabled in 1a8fb57, this patch addresses the race by
adding a second cluster version.
`EnablePebbleFormatVersionBlockProperties` acts as a barrier and a
feature gate. A guarantee of the migration framework is that any node at
this newer version is part of a cluster that has already run the
necessary migrations for the older version, and thus ratcheted the
format major version in the store, and thus enabled the block properties
feature, across all nodes.

Add additional documentation in `pebble.go` that details how to make use
of the two-version pattern for future table-level version changes.

Release note: None
With the introduction of block properties, and soon, range keys, which
introduce backward-incompatible changes at the SSTable level, all nodes
in a cluster must all have a sufficient store version in order to avoid
runtime incompatibilities.

Update `storage.MakeIngestionWriterOptions` to add a `context.Context`
and `cluster.Settings` as parameters, which allows for determining
whether a given cluster version is active (via
`(clusterversion.Handle).IsActive()`). This allows gating the enabling /
disabling of block properties (and soon, range keys), on all nodes being
at a sufficient cluster version.

Update various call-sites to pass in the `context.Context` and
`cluster.Settings`.

Closes cockroachdb#73768.

Release note: None
Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 28 files at r4, 27 of 27 files at r5, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dt and @erikgrinaker)

Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

LGTM. I suppose we'll have to do something similar across the board with all SST generation for range key support, right?

@nicktrav
Copy link
Collaborator Author

LGTM. I suppose we'll have to do something similar across the board with all SST generation for range key support, right?

Yessir. Next PR.

TFTRs!

bors r=dt,erikgrinaker,jbowens

@craig
Copy link
Contributor

craig bot commented Feb 15, 2022

Build failed (retrying...):

@craig craig bot merged commit 3cb7eb0 into cockroachdb:master Feb 15, 2022
@craig
Copy link
Contributor

craig bot commented Feb 15, 2022

Build succeeded:

@nicktrav nicktrav deleted the nickt.sst-writer-engine branch February 15, 2022 20:16
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.

5 participants