From 3d2f8e8b5b74ea7fee6cb4f23421b47ce4962f8c Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Sat, 22 Jul 2023 20:21:51 +0000 Subject: [PATCH] *: add a few example usages of `must` Epic: none Release note: None --- pkg/kv/kvserver/BUILD.bazel | 1 + pkg/kv/kvserver/batcheval/BUILD.bazel | 2 +- pkg/kv/kvserver/batcheval/cmd_clear_range.go | 90 ++++++++++---------- pkg/kv/kvserver/batcheval/cmd_export.go | 20 ++--- pkg/kv/kvserver/batcheval/cmd_migrate.go | 6 +- pkg/kv/kvserver/scheduler.go | 10 +-- pkg/roachpb/BUILD.bazel | 1 + pkg/roachpb/data.go | 10 +-- 8 files changed, 63 insertions(+), 77 deletions(-) diff --git a/pkg/kv/kvserver/BUILD.bazel b/pkg/kv/kvserver/BUILD.bazel index ded9ad9e4212..b3dfc8a2ef7e 100644 --- a/pkg/kv/kvserver/BUILD.bazel +++ b/pkg/kv/kvserver/BUILD.bazel @@ -206,6 +206,7 @@ go_library( "//pkg/util/metric", "//pkg/util/metric/aggmetric", "//pkg/util/mon", + "//pkg/util/must", "//pkg/util/pprofutil", "//pkg/util/protoutil", "//pkg/util/quotapool", diff --git a/pkg/kv/kvserver/batcheval/BUILD.bazel b/pkg/kv/kvserver/batcheval/BUILD.bazel index 5a38c4ec1998..54b9bc85b8f9 100644 --- a/pkg/kv/kvserver/batcheval/BUILD.bazel +++ b/pkg/kv/kvserver/batcheval/BUILD.bazel @@ -55,7 +55,6 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval", visibility = ["//visibility:public"], deps = [ - "//pkg/build", "//pkg/clusterversion", "//pkg/keys", "//pkg/kv/kvpb", @@ -85,6 +84,7 @@ go_library( "//pkg/util/limit", "//pkg/util/log", "//pkg/util/mon", + "//pkg/util/must", "//pkg/util/protoutil", "//pkg/util/tracing", "//pkg/util/uuid", diff --git a/pkg/kv/kvserver/batcheval/cmd_clear_range.go b/pkg/kv/kvserver/batcheval/cmd_clear_range.go index dfdbdcb0a76f..3b7aac49b16d 100644 --- a/pkg/kv/kvserver/batcheval/cmd_clear_range.go +++ b/pkg/kv/kvserver/batcheval/cmd_clear_range.go @@ -23,10 +23,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/storage/enginepb" - "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/must" "github.com/cockroachdb/errors" - "github.com/kr/pretty" ) // ClearRangeBytesThreshold is the threshold over which the ClearRange @@ -174,63 +173,60 @@ func computeStatsDelta( // We can avoid manually computing the stats delta if we're clearing // the entire range. - entireRange := desc.StartKey.Equal(from) && desc.EndKey.Equal(to) - if entireRange { + if desc.StartKey.Equal(from) && desc.EndKey.Equal(to) { // Note this it is safe to use the full range MVCC stats, as - // opposed to the usual method of computing only a localizied + // opposed to the usual method of computing only a localized // stats delta, because a full-range clear prevents any concurrent // access to the stats. Concurrent changes to range-local keys are // explicitly ignored (i.e. SysCount, SysBytes). delta = cArgs.EvalCtx.GetMVCCStats() delta.SysCount, delta.SysBytes, delta.AbortSpanBytes = 0, 0, 0 // no change to system stats - } - // If we can't use the fast stats path, or race test is enabled, compute stats - // across the key span to be cleared. - if !entireRange || util.RaceEnabled { - computed, err := storage.ComputeStats(readWriter, from, to, delta.LastUpdateNanos) + // Assert correct stats. + if err := must.Expensive(func() error { + if delta.ContainsEstimates != 0 { + return nil + } + computed, err := storage.ComputeStats(readWriter, from, to, delta.LastUpdateNanos) + if err != nil { + return err + } + return must.Equal(ctx, delta, computed, "range MVCC stats differ from computed") + }); err != nil { + return enginepb.MVCCStats{}, err + } + + } else { + // If we can't use the fast path, compute stats across the cleared span. + var err error + delta, err = storage.ComputeStats(readWriter, from, to, delta.LastUpdateNanos) if err != nil { return enginepb.MVCCStats{}, err } - // If we took the fast path but race is enabled, assert stats were correctly - // computed. - if entireRange { - // Retain the value of ContainsEstimates for tests under race. - computed.ContainsEstimates = delta.ContainsEstimates - // We only want to assert the correctness of stats that do not contain - // estimates. - if delta.ContainsEstimates == 0 && !delta.Equal(computed) { - log.Fatalf(ctx, "fast-path MVCCStats computation gave wrong result: diff(fast, computed) = %s", - pretty.Diff(delta, computed)) - } + + // We need to adjust for the fragmentation of any MVCC range tombstones that + // straddle the span bounds. The clearing of the inner fragments has already + // been accounted for above. We take care not to peek outside the Raft range + // bounds. + leftPeekBound, rightPeekBound := rangeTombstonePeekBounds( + from, to, desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey()) + rkIter := readWriter.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ + KeyTypes: storage.IterKeyTypeRangesOnly, + LowerBound: leftPeekBound, + UpperBound: rightPeekBound, + }) + defer rkIter.Close() + + if cmp, lhs, err := storage.PeekRangeKeysLeft(rkIter, from); err != nil { + return enginepb.MVCCStats{}, err + } else if cmp > 0 { + delta.Subtract(storage.UpdateStatsOnRangeKeySplit(from, lhs.Versions)) } - delta = computed - - // If we're not clearing the entire range, we need to adjust for the - // fragmentation of any MVCC range tombstones that straddle the span bounds. - // The clearing of the inner fragments has already been accounted for above. - // We take care not to peek outside the Raft range bounds. - if !entireRange { - leftPeekBound, rightPeekBound := rangeTombstonePeekBounds( - from, to, desc.StartKey.AsRawKey(), desc.EndKey.AsRawKey()) - rkIter := readWriter.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{ - KeyTypes: storage.IterKeyTypeRangesOnly, - LowerBound: leftPeekBound, - UpperBound: rightPeekBound, - }) - defer rkIter.Close() - - if cmp, lhs, err := storage.PeekRangeKeysLeft(rkIter, from); err != nil { - return enginepb.MVCCStats{}, err - } else if cmp > 0 { - delta.Subtract(storage.UpdateStatsOnRangeKeySplit(from, lhs.Versions)) - } - if cmp, rhs, err := storage.PeekRangeKeysRight(rkIter, to); err != nil { - return enginepb.MVCCStats{}, err - } else if cmp < 0 { - delta.Subtract(storage.UpdateStatsOnRangeKeySplit(to, rhs.Versions)) - } + if cmp, rhs, err := storage.PeekRangeKeysRight(rkIter, to); err != nil { + return enginepb.MVCCStats{}, err + } else if cmp < 0 { + delta.Subtract(storage.UpdateStatsOnRangeKeySplit(to, rhs.Versions)) } } diff --git a/pkg/kv/kvserver/batcheval/cmd_export.go b/pkg/kv/kvserver/batcheval/cmd_export.go index fd4e245a92f9..a2c7be6603bb 100644 --- a/pkg/kv/kvserver/batcheval/cmd_export.go +++ b/pkg/kv/kvserver/batcheval/cmd_export.go @@ -16,7 +16,6 @@ import ( "fmt" "time" - "github.com/cockroachdb/cockroach/pkg/build" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/batcheval/result" @@ -27,6 +26,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/must" "github.com/cockroachdb/cockroach/pkg/util/tracing" "github.com/cockroachdb/errors" ) @@ -274,19 +274,11 @@ func evalExport( reply.ResumeReason = kvpb.RESUME_ELASTIC_CPU_LIMIT break } else { - if !resumeInfo.CPUOverlimit { - // We should never come here. There should be no condition aside from - // resource constraints that results in an early exit without - // exporting any data. Regardless, if we have a resumeKey we - // immediately retry the ExportRequest from that key and timestamp - // onwards. - if !build.IsRelease() { - return result.Result{}, errors.AssertionFailedf("ExportRequest exited without " + - "exporting any data for an unknown reason; programming error") - } else { - log.Warningf(ctx, "unexpected resume span from ExportRequest without exporting any data for an unknown reason: %v", resumeInfo) - } - } + // There should be no condition aside from resource constraints that + // results in an early exit without exporting any data. Regardless, if + // we have a resumeKey we immediately retry the ExportRequest from + // that key and timestamp onwards. + _ = must.True(ctx, resumeInfo.CPUOverlimit, "Export returned no data: %+v", resumeInfo) start = resumeInfo.ResumeKey.Key resumeKeyTS = resumeInfo.ResumeKey.Timestamp continue diff --git a/pkg/kv/kvserver/batcheval/cmd_migrate.go b/pkg/kv/kvserver/batcheval/cmd_migrate.go index a0fc7d03ab59..c2f5f89a8abd 100644 --- a/pkg/kv/kvserver/batcheval/cmd_migrate.go +++ b/pkg/kv/kvserver/batcheval/cmd_migrate.go @@ -22,7 +22,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/spanset" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage" - "github.com/cockroachdb/errors" + "github.com/cockroachdb/cockroach/pkg/util/must" ) func init() { @@ -63,8 +63,8 @@ func Migrate( migrationVersion := args.Version fn, ok := migrationRegistry[migrationVersion] - if !ok { - return result.Result{}, errors.AssertionFailedf("migration for %s not found", migrationVersion) + if err := must.True(ctx, ok, "migration for %s not found", migrationVersion); err != nil { + return result.Result{}, err } pd, err := fn(ctx, readWriter, cArgs) if err != nil { diff --git a/pkg/kv/kvserver/scheduler.go b/pkg/kv/kvserver/scheduler.go index 06bc40b91d2d..ab60f8e980ab 100644 --- a/pkg/kv/kvserver/scheduler.go +++ b/pkg/kv/kvserver/scheduler.go @@ -18,8 +18,8 @@ import ( "unsafe" "github.com/cockroachdb/cockroach/pkg/roachpb" - "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/must" "github.com/cockroachdb/cockroach/pkg/util/stop" "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/cockroach/pkg/util/timeutil" @@ -400,11 +400,9 @@ func (ss *raftSchedulerShard) worker( state.flags |= stateRaftReady } } - if util.RaceEnabled { // assert the ticks invariant - if tick := state.flags&stateRaftTick != 0; tick != (state.ticks != 0) { - log.Fatalf(ctx, "stateRaftTick is %v with ticks %v", tick, state.ticks) - } - } + + _ = must.Equal(ctx, state.flags&stateRaftTick != 0, state.ticks != 0, + "flags %d with %d ticks", state.flags, state.ticks) // safe to continue if state.flags&stateRaftTick != 0 { for t := state.ticks; t > 0; t-- { // processRaftTick returns true if the range should perform ready diff --git a/pkg/roachpb/BUILD.bazel b/pkg/roachpb/BUILD.bazel index d8240ed0d63f..7a07e2462e02 100644 --- a/pkg/roachpb/BUILD.bazel +++ b/pkg/roachpb/BUILD.bazel @@ -38,6 +38,7 @@ go_library( "//pkg/util/humanizeutil", "//pkg/util/interval", "//pkg/util/log", + "//pkg/util/must", "//pkg/util/protoutil", "//pkg/util/syncutil", "//pkg/util/timetz", diff --git a/pkg/roachpb/data.go b/pkg/roachpb/data.go index a264fa07f7cd..b9bbce342039 100644 --- a/pkg/roachpb/data.go +++ b/pkg/roachpb/data.go @@ -32,13 +32,13 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock" "github.com/cockroachdb/cockroach/pkg/storage/enginepb" - "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/bitarray" "github.com/cockroachdb/cockroach/pkg/util/duration" "github.com/cockroachdb/cockroach/pkg/util/encoding" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/interval" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/must" "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/cockroach/pkg/util/timetz" "github.com/cockroachdb/cockroach/pkg/util/uuid" @@ -2464,11 +2464,9 @@ var _ sort.Interface = SequencedWriteBySeq{} // Find searches for the index of the SequencedWrite with the provided // sequence number. Returns -1 if no corresponding write is found. func (s SequencedWriteBySeq) Find(seq enginepb.TxnSeq) int { - if util.RaceEnabled { - if !sort.IsSorted(s) { - panic("SequencedWriteBySeq must be sorted") - } - } + _ = must.Expensive(func() error { + return must.True(context.TODO(), sort.IsSorted(s), "SequencedWriteBySeq not sorted") + }) if i := sort.Search(len(s), func(i int) bool { return s[i].Sequence >= seq }); i < len(s) && s[i].Sequence == seq {