cockroach icon indicating copy to clipboard operation
cockroach copied to clipboard

clusterversion,kvserver: remove 22.1 SpanConfig version gates

Open celiala opened this issue 2 years ago β€’ 4 comments

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

celiala avatar Aug 09 '22 20:08 celiala

This change is Reviewable

cockroach-teamcity avatar Aug 09 '22 20:08 cockroach-teamcity

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)

celiala avatar Aug 09 '22 22:08 celiala

This is on my list to look at tonight or tomorrow morning.

ajwerner avatar Aug 11 '22 00:08 ajwerner

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)

celiala avatar Aug 11 '22 00:08 celiala

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

celiala avatar Aug 15 '22 15:08 celiala

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?

arulajmani avatar Aug 19 '22 18:08 arulajmani

Yeah, I'm on board with deleting the test.

ajwerner avatar Aug 23 '22 00:08 ajwerner

@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!

arulajmani avatar Aug 23 '22 16:08 arulajmani

I took this over sent out a PR https://github.com/cockroachdb/cockroach/pull/86676 Thank you @arulajmani

celiala avatar Aug 23 '22 16:08 celiala

closing in favor of https://github.com/cockroachdb/cockroach/pull/86676

celiala avatar Aug 23 '22 16:08 celiala