clusterversion,storage: use range tombstones unconditionally in 23.1 #97770
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
nicktrav
requested review from
sumeerbhola,
jbowens,
msbutler and
erikgrinaker
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @erikgrinaker, @jbowens, @msbutler, @nicktrav, and @sumeerbhola)
pkg/storage/mvcc.go line 91 at r1 (raw file):
// V23_1_MVCCRangeTombstonesUnconditionallyEnabled, the feature is // unconditionally enabled. var MVCCRangeTombstonesEnabled = settings.RegisterBoolSetting(
[nit] maybe rename to MVCCRangeTombstonesEnabledInMixedClusters
pkg/storage/mvcc.go line 94 at r1 (raw file):
settings.TenantReadOnly, "storage.mvcc.range_tombstones.enabled", "enables the use of MVCC range tombstones",
[nit] some nuance can be added to this description. Maybe "controls the use of MVCC range tombstones in mixed version clusters; range tombstones are always on in 23.1 clusters".
|
Looks like there's just the one test failure. @msbutler - any thoughts on how you'd like |
nicktrav
force-pushed
the
nickt.mvcc-range-tombstones-version-gate
branch
from
7a6d0c1 to
f733158
Compare
|
this should do the trick! msbutler@9c45bd4 |
nicktrav
force-pushed
the
nickt.mvcc-range-tombstones-version-gate
branch
from
f733158 to
4dbf890
Compare
Indeed. Thanks! @msbutler - are y'all ok with the DR coverage as it stands in the "range tombstones unconditionaly on in 23.1" case? I'll defer to you as a follow up, if necessary. All other feedback addressed. |
|
from a DR perspective (sql client), i think test coverage is good! |
|
TFTR! bors r+ |
|
Build failed (retrying...): |
|
Build failed: |
|
bors retry |
|
Ick, this needs a rebase. bors cancel |
|
Canceled. |
nicktrav
force-pushed
the
nickt.mvcc-range-tombstones-version-gate
branch
from
4dbf890 to
c73f143
Compare
|
bors r+ |
|
Merge conflict. |
|
gah! rebase bors cancel |
Currently, MVCC range tombstones are only enabled when the `storage.mvcc.range_tombstones.enabled` cluster setting is `true`. Originally, the plan for 23.1 was to remove this cluster setting entirely, which would enable the feature unconditionally. However, it was decided that for 23.1, the setting would remain, along with the utility methods that determine whether or not the feature is enabled. As there is still an expectation that the feature is enabled in 23.1, add an internal cluster version that is used as a feature gate. If the cluster is at least of version `V23_1_MVCCRangeTombstonesUnconditionallyEnabled`, MVCC range tombstones are _unconditionally_ enabled, irrespective of whether the cluster setting is enabled or not. Fix cockroachdb#91147. Release note: None.
nicktrav
force-pushed
the
nickt.mvcc-range-tombstones-version-gate
branch
from
c73f143 to
1c3e46d
Compare
|
bors r+ |
|
Build failed: |
|
bors retry |
|
Build failed: |
|
bors retry |
|
Build succeeded: |
Currently, MVCC range tombstones are only enabled when the
storage.mvcc.range_tombstones.enabledcluster setting istrue. Originally, the plan for 23.1 was to remove this cluster setting entirely, which would enable the feature unconditionally. However, it was decided that for 23.1, the setting would remain, along with the utility methods that determine whether or not the feature is enabled.As there is still an expectation that the feature is enabled in 23.1, add an internal cluster version that is used as a feature gate. If the cluster is at least of version
V23_1_MVCCRangeTombstonesUnconditionallyEnabled, MVCC range tombstones are unconditionally enabled, irrespective of whether the cluster setting is enabled or not.Fix #91147.
Release note: None.
Epic: CRDB-20465.