sql,clusterversion,gc_job: hoist version gates for DelRange to 23.1#98228
Conversation
|
This change is |
fqazi
left a comment
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker)
pkg/sql/gcjob/gc_job.go
Outdated
| s.Version.IsActive(ctx, clusterversion.V23_1_UseDelRangeInGCJob) && | ||
| (storage.CanUseMVCCRangeTombstones(ctx, s) || |
There was a problem hiding this comment.
Shouldn't some of these predicates be somewhat different now? In mixed-version 22.2/23.1 clusters we should respect the storage.mvcc.range_tombstones.enabled setting until the V23_1_MVCCRangeTombstonesUnconditionallyEnabled version gate is enabled, at which point we should unconditionally use them. CanUseMVCCRangeTombstones() already encapsulates that logic.
The logic here will fail to respect storage.mvcc.range_tombstones.enabled in mixed-version 22.2/23.1 clusters.
There was a problem hiding this comment.
You're correct, it's done.
| @@ -508,7 +508,7 @@ func shouldUseDelRange( | |||
| ) bool { | |||
| // TODO(ajwerner): Adopt the DeleteRange protocol for tenant GC. | |||
There was a problem hiding this comment.
Apropos: will the fact that we don't use DeleteRange here cause problems for C2C streaming? Do we only replicate individual tenants? Do we simply cut off the stream when the tenant is destroyed, or otherwise remove it from the target out-of-band?
There was a problem hiding this comment.
@stevendanna can you answer this question?
There was a problem hiding this comment.
Do we only replicate individual tenants
Yes, we only replicate individual tenants.
Do we simply cut off the stream when the tenant is destroyed, or otherwise remove it from the target out-of-band?
If the tenant is dropped on the destination side, we stop the job. If the source side is dropped, I have a feeling we likely just keep going. However, an active replication job keeps protected timestamps alive on both the source and destination side. I believe the clear-range based protocol should wait out those protected timestamps before dropping the clears so it should be "OK" from that perspective. We may want a DROP on the source side to kill the producer side streaming job that owns that protected timestamp to prevent holding up GC for too long. But I'm not sure that is essential.
e0ed735 to
e8690f3
Compare
|
@erikgrinaker PTAL |
e8690f3 to
68d4daf
Compare
We added version gates to unconditionally enable sending DelRange tombstones in 22.2, but then we pre-empted that by disabling them with a cluster setting. This PR hoists those gates and that invariant checking up to 23.1. Relates to cockroachdb#96763 Release note: None
68d4daf to
05773b6
Compare
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
We added version gates to unconditionally enable sending DelRange tombstones in 22.2, but then we pre-empted that by disabling them with a cluster setting. This PR hoists those gates and that invariant checking up to 23.1.
Relates to #96763
Epic: None
Release note: None