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.

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

clusterversion,kvserver: remove 22.1 SpanConfig version gates #85848

Closed
wants to merge 1 commit into from

Conversation

celiala
Copy link
Collaborator

@celiala celiala commented Aug 9, 2022

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 #80663

Release note: none
Release justification: remove old version gates.

@celiala celiala requested review from a team as code owners August 9, 2022 20:20
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@celiala
Copy link
Collaborator Author

celiala commented Aug 9, 2022

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(?).
=== 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)

@ajwerner
Copy link
Contributor

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

@celiala
Copy link
Collaborator Author

celiala commented Aug 11, 2022

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 celiala force-pushed the remove-gates.SpanConfig branch 2 times, most recently from eee99a5 to 282ac82 Compare August 12, 2022 03:25
@celiala
Copy link
Collaborator Author

celiala commented Aug 15, 2022

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 celiala force-pushed the remove-gates.SpanConfig branch 2 times, most recently from 69939c5 to da94fe3 Compare August 19, 2022 01:52
@arulajmani
Copy link
Collaborator

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:

if !codec.ForSystemTenant() &&
(id == 0 || !version.IsActive(clusterversion.EnableSpanConfigStore)) {
codec, id = keys.SystemSQLCodec, keys.TenantsRangesID
}

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?

@celiala celiala force-pushed the remove-gates.SpanConfig branch from da94fe3 to 35f38f7 Compare August 22, 2022 21:55
@celiala celiala requested a review from a team as a code owner August 22, 2022 21:55
@ajwerner
Copy link
Contributor

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

@celiala celiala force-pushed the remove-gates.SpanConfig branch from 35f38f7 to ea67bef Compare August 23, 2022 01:44
Release note: none
Release justification: remove old version gates.
@celiala celiala force-pushed the remove-gates.SpanConfig branch from ea67bef to cbfed92 Compare August 23, 2022 02:00
arulajmani added a commit to arulajmani/cockroach that referenced this pull request Aug 23, 2022
Remove EnsureSPanConfigReconciliation, EnsureSpanConfigSubscription,
and EnableSpanConfigStore.

References cockroachdb#80663
Subsumes cockroachdb#85848

Release justification: cleanup
Release note: None
@arulajmani
Copy link
Collaborator

@celiala, like we talked about offline, I took this over sent out a PR #86676 with the failing test removed and some comment cleanup now that we don't have these version gates anymore. Thanks for driving this!

@celiala
Copy link
Collaborator Author

celiala commented Aug 23, 2022

I took this over sent out a PR #86676
Thank you @arulajmani

@celiala
Copy link
Collaborator Author

celiala commented Aug 23, 2022

closing in favor of #86676

@celiala celiala closed this Aug 23, 2022
arulajmani added a commit to arulajmani/cockroach that referenced this pull request Aug 23, 2022
Remove EnsureSPanConfigReconciliation, EnsureSpanConfigSubscription,
and EnableSpanConfigStore.

References cockroachdb#80663
Subsumes cockroachdb#85848

Release justification: cleanup
Release note: None
craig bot pushed a commit that referenced this pull request Aug 25, 2022
86436: kvserver: incorporate remote tracing spans from snapshots r=AlexTalks a=AlexTalks

This adds collected tracing spans into a `SnapshotResponse` object in
order to incorporate remote traces from the receiver side of a snapshot
into the client's (i.e. the sender's) context.

Release justification: Low-risk observability change.
Release note: None

86676: clusterversion, kvserver: remove SpanConfig related version gates r=celiala a=arulajmani

Remove EnsureSPanConfigReconciliation, EnsureSpanConfigSubscription,
and EnableSpanConfigStore.

References #80663
Subsumes #85848

Release justification: cleanup
Release note: None

Co-authored-by: Alex Sarkesian <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants