Add cluster version as feature gate for block properties#76278
Add cluster version as feature gate for block properties#76278craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
pkg/storage/metamorphic/generator.go
Outdated
| // 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
d44b1e7 to
d87d8f1
Compare
|
|
d87d8f1 to
2d66525
Compare
2d66525 to
4598090
Compare
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
4598090 to
6e2a057
Compare
jbowens
left a comment
There was a problem hiding this comment.
Reviewed 2 of 28 files at r4, 27 of 27 files at r5, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @dt and @erikgrinaker)
erikgrinaker
left a comment
There was a problem hiding this comment.
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 |
|
Build failed (retrying...): |
|
Build succeeded: |
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 notall 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.
EnablePebbleFormatVersionBlockPropertiesacts as a barrier and afeature 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.gothat details how to make useof 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.MakeIngestionWriterOptionsto add acontext.Contextand
cluster.Settingsas parameters, which allows for determiningwhether 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.Contextandcluster.Settings.