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.

Sign up for GitHub

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

Jump to bottom

storage: remove storage.mvcc.range_tombstones.enabled cluster setting #97869

Closed
nicktrav opened this issue Mar 1, 2023 · 6 comments
Closed

storage: remove storage.mvcc.range_tombstones.enabled cluster setting #97869

nicktrav opened this issue Mar 1, 2023 · 6 comments
Assignees
@jbowens
Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. T-storage Storage Team

Comments

@nicktrav
Copy link
Collaborator

nicktrav commented Mar 1, 2023 •
edited by cockroach-jira-scripts
Loading

Is your feature request related to a problem? Please describe.

The MVCC Range Tombstones feature shipped in 22.2, gated behind the storage.mvcc.range_tombstones.enabled cluster setting. While the setting will continue to exist in 23.1, the feature is unconditionally enabled (i.e. the cluster setting is ignored).

In 23.2 we should remove the cluster setting and the associated helper functions, etc.

Additional context

See #91147 for the original context.

Jira issue: CRDB-24933
@nicktrav nicktrav added C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. A-storage Relating to our storage engine (Pebble) on-disk storage. T-storage Storage Team labels Mar 1, 2023
jbowens added a commit to jbowens/cockroach that referenced this issue Aug 9, 2023
@jbowens
roachtest: remove rangeTs clearrange variants
6c636d8 
In clusters with finalized cluster versions ≥23.1, range tombstones are always
enabled and the cluster setting has no effect. Remove the variations of the
clearrange test, and we'll effectively always test with MVCC range tombstones.

Epic: none
Informs cockroachdb#97869.
Release note: none
@jbowens jbowens mentioned this issue Aug 9, 2023
Merged
craig bot pushed a commit that referenced this issue Aug 10, 2023
@jbowens
Merge #108487
00f1d4e 
108487: roachtest: remove rangeTs clearrange variants r=jbowens a=jbowens

In clusters with finalized cluster versions ≥23.1, range tombstones are always enabled and the cluster setting has no effect. Remove the variations of the clearrange test, and we'll effectively always test with MVCC range tombstones.

Epic: none
Informs #97869.
Release note: none

Co-authored-by: Jackson Owens <[email protected]>
@jbowens
Copy link
Collaborator

jbowens commented Aug 10, 2023

cockroach/pkg/ccl/backupccl/testdata/backup-restore/in-progress-import-rollback

Lines 10 to 13 in eff0185

# disabled to run within tenant as they don't have access to the
# storage.mvcc.range_tombstones.enabled cluster setting
new-cluster name=s1 beforeVersion=23_1_MVCCTombstones disable-tenant
----

@cockroachdb/disaster-recovery This unit test appears to still use this cluster setting. Can we delete or adjust this unit test to remove testing of non-MVCC rollbacks now?

@jbowens jbowens self-assigned this Aug 10, 2023
@msbutler
Copy link
Collaborator

Yes get rid of "case 1" in this test!

@dt
Copy link
Member

dt commented Aug 10, 2023

@msbutler might have more context here, but one question: 23.2 maintains backwards compatibility with 22.2 so I think we wanted to keep the tests that simulated restoring a 22.2 backup/the 22.2 range tombstone behavior?

@msbutler
Copy link
Collaborator

to do that, we'd need a 22.2 fixture which simulates the behavior. I think it's fine to delete this subtest to unblock this pr though.

jbowens added a commit to jbowens/cockroach that referenced this issue Aug 10, 2023
@jbowens
roachtest: remove rangeTs variants of import-cancellation test
0946522 
Previously the import-cancellation roachtest was split into two variants: one
with MVCC range tombstones enabled and one without. MVCC range tombstones are
always enabled now, so the two variants were effectively identical. This commit
consolidates the two tests into a single `import-cancellation` roachtest.

Informs cockroachdb#97869.
Epic: None
Release note: None
@jbowens jbowens mentioned this issue Aug 10, 2023
Merged
craig bot pushed a commit that referenced this issue Aug 10, 2023
@ericharmeling @rafiss @jbowens
Merge #107388 #108415 #108535
618c73a 
107388: metrics: refactor histogram bucket generation and testing r=ericharmeling a=ericharmeling

This commit refactors histogram bucketing for legibility and composibility. It also introduces a data-driven test for histogram bucket generation.

This refactor should make it easier to add additional metric categories, distributions, and bucket types.

Part of #97144.

Release note: None

108415: roachtest: deflake npgsql test r=rafiss a=rafiss

These upstream tests are flaky, so we ignore them.

informs #108414
fixes #108044
fixes #108504
Release note: None

108535: roachtest: remove rangeTs variants of import-cancellation test r=stevendanna a=jbowens

Previously the import-cancellation roachtest was split into two variants: one with MVCC range tombstones enabled and one without. MVCC range tombstones are always enabled now, so the two variants were effectively identical. This commit consolidates the two tests into a single `import-cancellation` roachtest.

Informs #97869.
Epic: None
Release note: None

Co-authored-by: Eric Harmeling <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>
jbowens added a commit to jbowens/cockroach that referenced this issue Aug 24, 2023
@jbowens
*: remove sites setting storage.mvcc.range_tombstones.enabled='t'
ff57d2d 
The storage.mvcc.range_tombstones.enabled cluster setting is ignored on 23.1+
cluster versions. On these cluster version, MVCC range tombstones are used
unconditionally. Remove test uses of this cluster setting that redundantly
set `storage.mvcc.range_tombstones.enabled` to true.

A few remaining tests disable the MVCC range tombstones cluster setting. These
tests will require some additional work to adapt.

Informs cockroachdb#97869.
Epic: none
Release note: none
jbowens added a commit to jbowens/cockroach that referenced this issue Aug 31, 2023
@jbowens
ccl,roachtest: remove uses of storage.mvcc.range_tombstones.enabled
fe6a83d 
Remove uses of the `storage.mvcc.range_tombstones.enabled` cluster setting.
This cluster setting is ignored in cluster version 23.1+.

Epic: none
Informs cockroachdb#97869.
Release note: none
@jbowens jbowens mentioned this issue Aug 31, 2023
Merged
craig bot pushed a commit that referenced this issue Sep 9, 2023
@jbowens
Merge #109823
b5836a5 
109823: ccl,roachtest: remove uses of storage.mvcc.range_tombstones.enabled r=bananabrick a=jbowens

Remove uses of the `storage.mvcc.range_tombstones.enabled` cluster setting. This cluster setting is ignored in cluster version 23.1+.

Epic: none
Informs #97869.
Release note: none

Co-authored-by: Jackson Owens <[email protected]>
@itsbilal
Copy link
Member

itsbilal commented Feb 6, 2024

This cluster setting has been removed already.

@itsbilal itsbilal closed this as completed Feb 6, 2024
@itsbilal
Copy link
Member

For reference: this was removed in #112269.

@jbowens jbowens moved this to Done in [Deprecated] Storage Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jbowens jbowens

Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. T-storage Storage Team
Projects
[Deprecated] Storage
Archived in project
Milestone
No milestone
Development

No branches or pull requests
5 participants
@dt @itsbilal @nicktrav @jbowens @msbutler