Skip to content

Commit

Permalink
clusterversion,kv: remove CPutInline
Browse files Browse the repository at this point in the history
Partially addresses cockroachdb#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
  • Loading branch information
nvanbenschoten committed Sep 2, 2021
1 parent 0cb53f3 commit 99157d9
Show file tree
Hide file tree
Showing 9 changed files with 52 additions and 91 deletions.
6 changes: 0 additions & 6 deletions pkg/clusterversion/cockroach_versions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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},
Expand Down
87 changes: 43 additions & 44 deletions pkg/clusterversion/key_string.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion pkg/kv/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ go_library(
visibility = ["//visibility:public"],
deps = [
"//pkg/base",
"//pkg/clusterversion",
"//pkg/keys",
"//pkg/kv/kvbase",
"//pkg/roachpb:with-mocks",
Expand Down
12 changes: 2 additions & 10 deletions pkg/kv/batch.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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)
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/kv/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down
23 changes: 1 addition & 22 deletions pkg/kv/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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)
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/kv/db_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 0 additions & 1 deletion pkg/server/status/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ go_library(
visibility = ["//visibility:public"],
deps = [
"//pkg/build",
"//pkg/clusterversion",
"//pkg/gossip",
"//pkg/keys",
"//pkg/kv",
Expand Down
5 changes: 2 additions & 3 deletions pkg/server/status/recorder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -512,15 +511,15 @@ 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
}
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")
Expand Down

0 comments on commit 99157d9

Please sign in to comment.