cockroach
cockroach copied to clipboard
clusterversion,kvserver: remove 22.1 SpanConfig version gates
This commit removes the following 22.1 version gates:
- EnsureSpanConfigReconciliation
- EnsureSpanConfigSubscription
- EnableSpanConfigStore
Cleanup was done following guidance from 21.2 cleanup:
For the most part, if the gates were just simple if !version.IsActive { return x } or something, I just removed the block, and even if it was a little more complicated, like args = [x]; if version { args = append(args, y) }; foo(args) I still tried to mostly inline it such that it looked natural (i.e. remove that append and make it args = [x, y]).
However for just a couple more complicated cases that were referring to <21.2 versions that needed to be replaced when those were deleted, I added a placeholder clusterversion.TODOPre21_2 alias for 21.2. Replacing those calls with this alias shouldn't change their behavior -- it was already always true, since the code today should never run in a <21.2 cluster -- but means we can delete those older versions in the meantime and then the owners of these bits can decide how to update them.
Partially addresses https://github.com/cockroachdb/cockroach/issues/80663
Release note: none
Digging into failing tests:
-
pkg/sql/logictest/tests/fakedist/fakedist_test
is a known flake. -
github.com/cockroachdb/cockroach/pkg/ccl/kvccl/kvtenantccl: TestTenantSystemConfigUpgrade
seems to be legit fail (see below output). Although, it's not clear to me how this change cause the below error(?).- maybe @ajwerner or @irfansharif have suggestions on how to fix?
=== RUN TestTenantSystemConfigUpgrade
test_log_scope.go:162: test logs captured to: /go/src/github.com/cockroachdb/cockroach/artifacts/logTestTenantSystemConfigUpgrade2787488539
test_log_scope.go:80: use -show-logs to present logs inline
tenant_upgrade_test.go:457: condition failed to evaluate within 45s: got 90000, expected 111
panic.go:482: -- test log scope end --
test logs left over in: /go/src/github.com/cockroachdb/cockroach/artifacts/logTestTenantSystemConfigUpgrade2787488539
--- FAIL: TestTenantSystemConfigUpgrade (61.10s)
This is on my list to look at tonight or tomorrow morning.
There's now a net new test failure (didn't fail on previous runs?):
github.com/cockroachdb/cockroach/pkg/sql/ttl/ttljob
:
TestRowLevelTTLJobRandomEntries/one_column_pk_multiple_nodes
------- Stdout: -------
=== RUN TestRowLevelTTLJobRandomEntries/one_column_pk_multiple_nodes
ttljob_test.go:546: test case: ttljob_test.testCase{desc:"one column pk multiple nodes", createTable:"CREATE TABLE tbl (\n\tid UUID PRIMARY KEY DEFAULT gen_random_uuid(),\n\ttext TEXT\n) WITH (ttl_expire_after = '30 days')", preSetup:[]string(nil), postSetup:[]string(nil), numExpiredRows:1001, numNonExpiredRows:5, numSplits:10, forceNonMultiTenant:false, numNodes:5, expirationExpression:"", addRow:(func(*ttljob_test.rowLevelTTLTestJobTestHelper, *tree.CreateTable, time.Time))(nil)}
ttljob_test.go:679:
Error Trace: ttljob_test.go:679
Error: Not equal:
expected: 1001
actual : 1072
Test: TestRowLevelTTLJobRandomEntries/one_column_pk_multiple_nodes
--- FAIL: TestRowLevelTTLJobRandomEntries/one_column_pk_multiple_nodes (18.26s)
These tests seem to be consistently failing, could use some help fixing π (feel free to take-over/add to this PR as needed, if easier :) )
π΄ TestAdminRelocateRange (maybe @tobias?)
client_relocate_range_test.go:238:
Error Trace: client_relocate_range_test.go:189
client_relocate_range_test.go:238
Error: Not equal:
expected: 3
actual : 2
π΄ TestTenantSystemConfigUpgrade (maybe @werner?)
- tenant_upgrade_test.go:457:
- condition failed to evaluate within 45s: got 90000, expected 111
β pkg/ccl/spanconfigccl/spanconfiglimiterccl/spanconfiglimiterccl_test_/spanconfiglimiterccl_test ` code 142
π΄ TestAtomicReplicationChange (maybe @tobias as well?)
- No pass/skip/fail event found for test
I briefly looked at the TestTenantSystemConfigUpgrade
failure and it makes sense -- it was exercising this code path, but we no longer need/have a cluster version check here:
https://github.com/cockroachdb/cockroach/blob/677ef2c6b7f0accefce2b1b7de52902e0e0be984/pkg/config/system.go#L421-L424
We could update the test a bit, but I'm not sure if there's much value keeping it around given what it intended to test. Maybe we should just delete it? @ajwerner what do you think?
Yeah, I'm on board with deleting the test.
@celiala, like we talked about offline, I took this over sent out a PR https://github.com/cockroachdb/cockroach/pull/86676 with the failing test removed and some comment cleanup now that we don't have these version gates anymore. Thanks for driving this!
I took this over sent out a PR https://github.com/cockroachdb/cockroach/pull/86676 Thank you @arulajmani
closing in favor of https://github.com/cockroachdb/cockroach/pull/86676