Skip to content

Commit

Permalink
storage: add version check to IsSeparatedIntentsEnabledForTesting
Browse files Browse the repository at this point in the history
`Engine.IsSeparatedIntentsEnabledForTesting()` is a test method to
detect whether separated intents are enabled (as these are randomly
enabled during tests). However, the function only checked the setting,
and failed to check whether the cluster version was recent enough,
causing it to return false positives.

This patch adds a cluster version check to fix this, which also required
changing the interface to thread a context through.

Release note: None
  • Loading branch information
erikgrinaker committed Mar 12, 2021
1 parent c7f9851 commit 0c1c6d9
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 11 deletions.
3 changes: 2 additions & 1 deletion pkg/kv/kvserver/replica_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6245,6 +6245,7 @@ func TestRangeStatsComputation(t *testing.T) {
stopper := stop.NewStopper()
defer stopper.Stop(context.Background())
tc.Start(t, stopper)
ctx := context.Background()

baseStats := initialStats()
// The initial stats contain an empty lease and no prior read summary, but
Expand Down Expand Up @@ -6311,7 +6312,7 @@ func TestRangeStatsComputation(t *testing.T) {
// Account for TxnDidNotUpdateMeta
expMS.LiveBytes += 2
expMS.ValBytes += 2
if tc.engine.IsSeparatedIntentsEnabledForTesting() {
if tc.engine.IsSeparatedIntentsEnabledForTesting(ctx) {
expMS.SeparatedIntentCount++
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/storage/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -715,7 +715,7 @@ type Engine interface {
// IsSeparatedIntentsEnabledForTesting is a test only method used in tests
// that know that this enabled setting is not changing and need the value to
// adjust their expectations.
IsSeparatedIntentsEnabledForTesting() bool
IsSeparatedIntentsEnabledForTesting(ctx context.Context) bool
}

// Batch is the interface for batch specific operations.
Expand Down
14 changes: 7 additions & 7 deletions pkg/storage/mvcc_stats_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ func TestMVCCStatsPutCommitMovesTimestamp(t *testing.T) {
if accountForTxnDidNotUpdateMeta(t, engine) {
// Account for TxnDidNotUpdateMeta
mValSize += 2
if engine.IsSeparatedIntentsEnabledForTesting() {
if engine.IsSeparatedIntentsEnabledForTesting(ctx) {
separatedIntentCount = 1
}
}
Expand Down Expand Up @@ -289,7 +289,7 @@ func TestMVCCStatsPutPushMovesTimestamp(t *testing.T) {
if accountForTxnDidNotUpdateMeta(t, engine) {
// Account for TxnDidNotUpdateMeta
mValSize += 2
if engine.IsSeparatedIntentsEnabledForTesting() {
if engine.IsSeparatedIntentsEnabledForTesting(ctx) {
separatedIntentCount = 1
}
}
Expand Down Expand Up @@ -391,7 +391,7 @@ func TestMVCCStatsDeleteMovesTimestamp(t *testing.T) {
if accountForTxnDidNotUpdateMeta(t, engine) {
// Account for TxnDidNotUpdateMeta
mVal1Size += 2
if engine.IsSeparatedIntentsEnabledForTesting() {
if engine.IsSeparatedIntentsEnabledForTesting(ctx) {
separatedIntentCount = 1
}
}
Expand Down Expand Up @@ -521,7 +521,7 @@ func TestMVCCStatsPutMovesDeletionTimestamp(t *testing.T) {
if accountForTxnDidNotUpdateMeta(t, engine) {
// Account for TxnDidNotUpdateMeta
mVal1Size += 2
if engine.IsSeparatedIntentsEnabledForTesting() {
if engine.IsSeparatedIntentsEnabledForTesting(ctx) {
separatedIntentCount = 1
}
}
Expand Down Expand Up @@ -657,7 +657,7 @@ func TestMVCCStatsDelDelCommitMovesTimestamp(t *testing.T) {
if accountForTxnDidNotUpdateMeta(t, engine) {
// Account for TxnDidNotUpdateMeta
mValSize += 2
if engine.IsSeparatedIntentsEnabledForTesting() {
if engine.IsSeparatedIntentsEnabledForTesting(ctx) {
separatedIntentCount = 1
}
}
Expand Down Expand Up @@ -813,7 +813,7 @@ func TestMVCCStatsPutDelPutMovesTimestamp(t *testing.T) {
if accountForTxnDidNotUpdateMeta(t, engine) {
// Account for TxnDidNotUpdateMeta
mValSize += 2
if engine.IsSeparatedIntentsEnabledForTesting() {
if engine.IsSeparatedIntentsEnabledForTesting(ctx) {
separatedIntentCount = 1
}
}
Expand Down Expand Up @@ -1040,7 +1040,7 @@ func TestMVCCStatsPutIntentTimestampNotPutTimestamp(t *testing.T) {
if accountForTxnDidNotUpdateMeta(t, engine) {
// Account for TxnDidNotUpdateMeta
m1ValSize += 2
if engine.IsSeparatedIntentsEnabledForTesting() {
if engine.IsSeparatedIntentsEnabledForTesting(ctx) {
separatedIntentCount = 1
}
}
Expand Down
6 changes: 4 additions & 2 deletions pkg/storage/pebble.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"time"

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/settings"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
Expand Down Expand Up @@ -900,8 +901,9 @@ func (p *Pebble) SafeToWriteSeparatedIntents(ctx context.Context) (bool, error)
}

// IsSeparatedIntentsEnabledForTesting implements the Engine interface.
func (p *Pebble) IsSeparatedIntentsEnabledForTesting() bool {
return SeparatedIntentsEnabled.Get(&p.settings.SV)
func (p *Pebble) IsSeparatedIntentsEnabledForTesting(ctx context.Context) bool {
return !p.settings.Version.ActiveVersionOrEmpty(ctx).Less(
clusterversion.ByKey(clusterversion.SeparatedIntents)) && SeparatedIntentsEnabled.Get(&p.settings.SV)
}

func (p *Pebble) put(key MVCCKey, value []byte) error {
Expand Down

0 comments on commit 0c1c6d9

Please sign in to comment.