From 99157d973ab194ae4db160392fda21e89bfce91d Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Wed, 1 Sep 2021 23:51:41 -0400 Subject: [PATCH] clusterversion,kv: remove CPutInline Partially addresses #66544 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release justification: cluster version cleanup --- pkg/clusterversion/cockroach_versions.go | 6 -- pkg/clusterversion/key_string.go | 87 ++++++++++++------------ pkg/kv/BUILD.bazel | 1 - pkg/kv/batch.go | 12 +--- pkg/kv/client_test.go | 4 +- pkg/kv/db.go | 23 +------ pkg/kv/db_test.go | 4 +- pkg/server/status/BUILD.bazel | 1 - pkg/server/status/recorder.go | 5 +- 9 files changed, 52 insertions(+), 91 deletions(-) diff --git a/pkg/clusterversion/cockroach_versions.go b/pkg/clusterversion/cockroach_versions.go index 282e6354a979..e738e91b608e 100644 --- a/pkg/clusterversion/cockroach_versions.go +++ b/pkg/clusterversion/cockroach_versions.go @@ -180,8 +180,6 @@ const ( // // Start21_1 demarcates work towards CockroachDB v21.1. Start21_1 - // CPutInline is conditional put support for inline values. - CPutInline // ReplicaVersions enables the versioning of Replica state. ReplicaVersions // replacedTruncatedAndRangeAppliedStateMigration stands in for @@ -361,10 +359,6 @@ var versionsSingleton = keyedVersions{ Key: Start21_1, Version: roachpb.Version{Major: 20, Minor: 2, Internal: 2}, }, - { - Key: CPutInline, - Version: roachpb.Version{Major: 20, Minor: 2, Internal: 10}, - }, { Key: ReplicaVersions, Version: roachpb.Version{Major: 20, Minor: 2, Internal: 12}, diff --git a/pkg/clusterversion/key_string.go b/pkg/clusterversion/key_string.go index 819cf5630a11..6e10593b5d4a 100644 --- a/pkg/clusterversion/key_string.go +++ b/pkg/clusterversion/key_string.go @@ -14,53 +14,52 @@ func _() { _ = x[HBAForNonTLS-3] _ = x[V20_2-4] _ = x[Start21_1-5] - _ = x[CPutInline-6] - _ = x[ReplicaVersions-7] - _ = x[replacedTruncatedAndRangeAppliedStateMigration-8] - _ = x[replacedPostTruncatedAndRangeAppliedStateMigration-9] - _ = x[TruncatedAndRangeAppliedStateMigration-10] - _ = x[PostTruncatedAndRangeAppliedStateMigration-11] - _ = x[SeparatedIntents-12] - _ = x[TracingVerbosityIndependentSemantics-13] - _ = x[PriorReadSummaries-14] - _ = x[NonVotingReplicas-15] - _ = x[V21_1-16] - _ = x[Start21_1PLUS-17] - _ = x[Start21_2-18] - _ = x[JoinTokensTable-19] - _ = x[AcquisitionTypeInLeaseHistory-20] - _ = x[SerializeViewUDTs-21] - _ = x[ExpressionIndexes-22] - _ = x[DeleteDeprecatedNamespaceTableDescriptorMigration-23] - _ = x[FixDescriptors-24] - _ = x[SQLStatsTable-25] - _ = x[DatabaseRoleSettings-26] - _ = x[TenantUsageTable-27] - _ = x[SQLInstancesTable-28] - _ = x[NewRetryableRangefeedErrors-29] - _ = x[AlterSystemWebSessionsCreateIndexes-30] - _ = x[SeparatedIntentsMigration-31] - _ = x[PostSeparatedIntentsMigration-32] - _ = x[RetryJobsWithExponentialBackoff-33] - _ = x[RecordsBasedRegistry-34] - _ = x[AutoSpanConfigReconciliationJob-35] - _ = x[PreventNewInterleavedTables-36] - _ = x[EnsureNoInterleavedTables-37] - _ = x[DefaultPrivileges-38] - _ = x[ZonesTableForSecondaryTenants-39] - _ = x[UseKeyEncodeForHashShardedIndexes-40] - _ = x[DatabasePlacementPolicy-41] - _ = x[GeneratedAsIdentity-42] - _ = x[OnUpdateExpressions-43] - _ = x[SpanConfigurationsTable-44] - _ = x[BoundedStaleness-45] - _ = x[SQLStatsCompactionScheduledJob-46] - _ = x[DateAndIntervalStyle-47] + _ = x[ReplicaVersions-6] + _ = x[replacedTruncatedAndRangeAppliedStateMigration-7] + _ = x[replacedPostTruncatedAndRangeAppliedStateMigration-8] + _ = x[TruncatedAndRangeAppliedStateMigration-9] + _ = x[PostTruncatedAndRangeAppliedStateMigration-10] + _ = x[SeparatedIntents-11] + _ = x[TracingVerbosityIndependentSemantics-12] + _ = x[PriorReadSummaries-13] + _ = x[NonVotingReplicas-14] + _ = x[V21_1-15] + _ = x[Start21_1PLUS-16] + _ = x[Start21_2-17] + _ = x[JoinTokensTable-18] + _ = x[AcquisitionTypeInLeaseHistory-19] + _ = x[SerializeViewUDTs-20] + _ = x[ExpressionIndexes-21] + _ = x[DeleteDeprecatedNamespaceTableDescriptorMigration-22] + _ = x[FixDescriptors-23] + _ = x[SQLStatsTable-24] + _ = x[DatabaseRoleSettings-25] + _ = x[TenantUsageTable-26] + _ = x[SQLInstancesTable-27] + _ = x[NewRetryableRangefeedErrors-28] + _ = x[AlterSystemWebSessionsCreateIndexes-29] + _ = x[SeparatedIntentsMigration-30] + _ = x[PostSeparatedIntentsMigration-31] + _ = x[RetryJobsWithExponentialBackoff-32] + _ = x[RecordsBasedRegistry-33] + _ = x[AutoSpanConfigReconciliationJob-34] + _ = x[PreventNewInterleavedTables-35] + _ = x[EnsureNoInterleavedTables-36] + _ = x[DefaultPrivileges-37] + _ = x[ZonesTableForSecondaryTenants-38] + _ = x[UseKeyEncodeForHashShardedIndexes-39] + _ = x[DatabasePlacementPolicy-40] + _ = x[GeneratedAsIdentity-41] + _ = x[OnUpdateExpressions-42] + _ = x[SpanConfigurationsTable-43] + _ = x[BoundedStaleness-44] + _ = x[SQLStatsCompactionScheduledJob-45] + _ = x[DateAndIntervalStyle-46] } -const _Key_name = "Start20_2MinPasswordLengthCreateLoginPrivilegeHBAForNonTLSV20_2Start21_1CPutInlineReplicaVersionsreplacedTruncatedAndRangeAppliedStateMigrationreplacedPostTruncatedAndRangeAppliedStateMigrationTruncatedAndRangeAppliedStateMigrationPostTruncatedAndRangeAppliedStateMigrationSeparatedIntentsTracingVerbosityIndependentSemanticsPriorReadSummariesNonVotingReplicasV21_1Start21_1PLUSStart21_2JoinTokensTableAcquisitionTypeInLeaseHistorySerializeViewUDTsExpressionIndexesDeleteDeprecatedNamespaceTableDescriptorMigrationFixDescriptorsSQLStatsTableDatabaseRoleSettingsTenantUsageTableSQLInstancesTableNewRetryableRangefeedErrorsAlterSystemWebSessionsCreateIndexesSeparatedIntentsMigrationPostSeparatedIntentsMigrationRetryJobsWithExponentialBackoffRecordsBasedRegistryAutoSpanConfigReconciliationJobPreventNewInterleavedTablesEnsureNoInterleavedTablesDefaultPrivilegesZonesTableForSecondaryTenantsUseKeyEncodeForHashShardedIndexesDatabasePlacementPolicyGeneratedAsIdentityOnUpdateExpressionsSpanConfigurationsTableBoundedStalenessSQLStatsCompactionScheduledJobDateAndIntervalStyle" +const _Key_name = "Start20_2MinPasswordLengthCreateLoginPrivilegeHBAForNonTLSV20_2Start21_1ReplicaVersionsreplacedTruncatedAndRangeAppliedStateMigrationreplacedPostTruncatedAndRangeAppliedStateMigrationTruncatedAndRangeAppliedStateMigrationPostTruncatedAndRangeAppliedStateMigrationSeparatedIntentsTracingVerbosityIndependentSemanticsPriorReadSummariesNonVotingReplicasV21_1Start21_1PLUSStart21_2JoinTokensTableAcquisitionTypeInLeaseHistorySerializeViewUDTsExpressionIndexesDeleteDeprecatedNamespaceTableDescriptorMigrationFixDescriptorsSQLStatsTableDatabaseRoleSettingsTenantUsageTableSQLInstancesTableNewRetryableRangefeedErrorsAlterSystemWebSessionsCreateIndexesSeparatedIntentsMigrationPostSeparatedIntentsMigrationRetryJobsWithExponentialBackoffRecordsBasedRegistryAutoSpanConfigReconciliationJobPreventNewInterleavedTablesEnsureNoInterleavedTablesDefaultPrivilegesZonesTableForSecondaryTenantsUseKeyEncodeForHashShardedIndexesDatabasePlacementPolicyGeneratedAsIdentityOnUpdateExpressionsSpanConfigurationsTableBoundedStalenessSQLStatsCompactionScheduledJobDateAndIntervalStyle" -var _Key_index = [...]uint16{0, 9, 26, 46, 58, 63, 72, 82, 97, 143, 193, 231, 273, 289, 325, 343, 360, 365, 378, 387, 402, 431, 448, 465, 514, 528, 541, 561, 577, 594, 621, 656, 681, 710, 741, 761, 792, 819, 844, 861, 890, 923, 946, 965, 984, 1007, 1023, 1053, 1073} +var _Key_index = [...]uint16{0, 9, 26, 46, 58, 63, 72, 87, 133, 183, 221, 263, 279, 315, 333, 350, 355, 368, 377, 392, 421, 438, 455, 504, 518, 531, 551, 567, 584, 611, 646, 671, 700, 731, 751, 782, 809, 834, 851, 880, 913, 936, 955, 974, 997, 1013, 1043, 1063} func (i Key) String() string { if i < 0 || i >= Key(len(_Key_index)-1) { diff --git a/pkg/kv/BUILD.bazel b/pkg/kv/BUILD.bazel index 721830635144..e353d9b3d054 100644 --- a/pkg/kv/BUILD.bazel +++ b/pkg/kv/BUILD.bazel @@ -17,7 +17,6 @@ go_library( visibility = ["//visibility:public"], deps = [ "//pkg/base", - "//pkg/clusterversion", "//pkg/keys", "//pkg/kv/kvbase", "//pkg/roachpb:with-mocks", diff --git a/pkg/kv/batch.go b/pkg/kv/batch.go index 2f0295bd07be..88e72b44f20a 100644 --- a/pkg/kv/batch.go +++ b/pkg/kv/batch.go @@ -13,7 +13,6 @@ package kv import ( "context" - "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage/enginepb" "github.com/cockroachdb/cockroach/pkg/util/hlc" @@ -464,7 +463,7 @@ func (b *Batch) CPutAllowingIfNotExists(key, value interface{}, expValue []byte) b.cputInternal(key, value, expValue, true, false) } -// cPutInline conditionally sets the value for a key if the existing value is +// CPutInline conditionally sets the value for a key if the existing value is // equal to expValue, but does not maintain multi-version values. To // conditionally set a value only if the key doesn't currently exist, pass an // empty expValue. The most recent value is always overwritten. Inline values @@ -478,14 +477,7 @@ func (b *Batch) CPutAllowingIfNotExists(key, value interface{}, expValue []byte) // // A nil value can be used to delete the respective key, since there is no // DelInline(). This is different from CPut(). -// -// Callers should check the version gate clusterversion.CPutInline to make sure -// this is supported. The method is unexported to prevent external callers using -// this without checking the version, since the CtxForCPutInline guard can't be -// used with Batch. -func (b *Batch) cPutInline(key, value interface{}, expValue []byte) { - // TODO(erikgrinaker): export once clusterversion.CPutInline is removed. - _ = clusterversion.CPutInline +func (b *Batch) CPutInline(key, value interface{}, expValue []byte) { b.cputInternal(key, value, expValue, false, true) } diff --git a/pkg/kv/client_test.go b/pkg/kv/client_test.go index d2a6052377b9..a13a209df339 100644 --- a/pkg/kv/client_test.go +++ b/pkg/kv/client_test.go @@ -369,10 +369,10 @@ func TestClientPutInline(t *testing.T) { func TestClientCPutInline(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + ctx := context.Background() s, _, _ := serverutils.StartServer(t, base.TestServerArgs{}) - defer s.Stopper().Stop(context.Background()) + defer s.Stopper().Stop(ctx) db := createTestClient(t, s) - ctx := kv.CtxForCPutInline(context.Background()) key := testUser + "/key" value := []byte("value") diff --git a/pkg/kv/db.go b/pkg/kv/db.go index fc2021cbb9dc..d166e8c45ac5 100644 --- a/pkg/kv/db.go +++ b/pkg/kv/db.go @@ -16,7 +16,6 @@ import ( "strings" "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage/enginepb" "github.com/cockroachdb/cockroach/pkg/util/admission" @@ -409,18 +408,6 @@ func (db *DB) CPut(ctx context.Context, key, value interface{}, expValue []byte) return getOneErr(db.Run(ctx, b), b) } -// CtxForCPutInline is a gate to make sure the caller is aware that CPutInline -// is only available with clusterversion.CPutInline, and must check this before -// using the method. -func CtxForCPutInline(ctx context.Context) context.Context { - // TODO(erikgrinaker): This code and all of its uses can be removed when the - // version below is removed: - _ = clusterversion.CPutInline - return context.WithValue(ctx, canUseCPutInline{}, canUseCPutInline{}) -} - -type canUseCPutInline struct{} - // CPutInline conditionally sets the value for a key if the existing value is // equal to expValue, but does not maintain multi-version values. To // conditionally set a value only if the key doesn't currently exist, pass an @@ -436,17 +423,9 @@ type canUseCPutInline struct{} // An empty expValue means that the key is expected to not exist. If not empty, // expValue needs to correspond to a Value.TagAndDataBytes() - i.e. a key's // value without the checksum (as the checksum includes the key too). -// -// Callers should check the version gate clusterversion.CPutInline to make sure -// this is supported, and must wrap the context using CtxForCPutInline(ctx) to -// enable the call. func (db *DB) CPutInline(ctx context.Context, key, value interface{}, expValue []byte) error { - if ctx.Value(canUseCPutInline{}) == nil { - return errors.New("CPutInline is new in 21.1, you must check the CPutInline cluster version " + - "and use CtxForCPutInline to enable it") - } b := &Batch{} - b.cPutInline(key, value, expValue) + b.CPutInline(key, value, expValue) return getOneErr(db.Run(ctx, b), b) } diff --git a/pkg/kv/db_test.go b/pkg/kv/db_test.go index 5c418126739e..ab2472013ece 100644 --- a/pkg/kv/db_test.go +++ b/pkg/kv/db_test.go @@ -170,9 +170,9 @@ func TestDB_CPut(t *testing.T) { func TestDB_CPutInline(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) + ctx := context.Background() s, db := setup(t) - defer s.Stopper().Stop(context.Background()) - ctx := kv.CtxForCPutInline(context.Background()) + defer s.Stopper().Stop(ctx) if err := db.PutInline(ctx, "aa", "1"); err != nil { t.Fatal(err) diff --git a/pkg/server/status/BUILD.bazel b/pkg/server/status/BUILD.bazel index d06999942b8d..94ddbf15fc4f 100644 --- a/pkg/server/status/BUILD.bazel +++ b/pkg/server/status/BUILD.bazel @@ -40,7 +40,6 @@ go_library( visibility = ["//visibility:public"], deps = [ "//pkg/build", - "//pkg/clusterversion", "//pkg/gossip", "//pkg/keys", "//pkg/kv", diff --git a/pkg/server/status/recorder.go b/pkg/server/status/recorder.go index eaf95ff5f274..80572356f5b1 100644 --- a/pkg/server/status/recorder.go +++ b/pkg/server/status/recorder.go @@ -24,7 +24,6 @@ import ( "time" "github.com/cockroachdb/cockroach/pkg/build" - "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/gossip" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" @@ -512,7 +511,7 @@ func (mr *MetricsRecorder) WriteNodeStatus( // of the build info in the node status, writing one of these every 10s // will generate more versions than will easily fit into a range over // the course of a day. - if mustExist && mr.settings.Version.IsActive(ctx, clusterversion.CPutInline) { + if mustExist { entry, err := db.Get(ctx, key) if err != nil { return err @@ -520,7 +519,7 @@ func (mr *MetricsRecorder) WriteNodeStatus( if entry.Value == nil { return errors.New("status entry not found, node may have been decommissioned") } - err = db.CPutInline(kv.CtxForCPutInline(ctx), key, &nodeStatus, entry.Value.TagAndDataBytes()) + err = db.CPutInline(ctx, key, &nodeStatus, entry.Value.TagAndDataBytes()) if detail := (*roachpb.ConditionFailedError)(nil); errors.As(err, &detail) { if detail.ActualValue == nil { return errors.New("status entry not found, node may have been decommissioned")