Skip to content
Permalink

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: cockroachdb/cockroach
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: master
Choose a base ref
...
head repository: wenyihu6/cockroach
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: master
Choose a head ref
Can’t automatically merge. Don’t worry, you can still create the pull request.
  • 1 commit
  • 4 files changed
  • 1 contributor

Commits on May 17, 2023

  1. kvserver: annotate queues with profiler labels

    Prior to this commit, different types of queues (consistency, merge, replica…) reuse baseQueue as the base
    implementation for queues. But goroutine dump does not explicitly show which queue called the function specifically
    which makes debugging complicated. This commit adds every queue’s name as a log tag to some queue implementation
    interfaces including maybeAdd and processLoop.
    wenyihu6 committed May 17, 2023
    Copy the full SHA
    5cd58f7 View commit details
Showing with 57 additions and 1 deletion.
  1. +1 −0 pkg/kv/kvserver/BUILD.bazel
  2. +16 −1 pkg/kv/kvserver/queue.go
  3. +37 −0 pkg/kv/kvserver/queue_test.go
  4. +3 −0 pkg/kv/kvserver/testing_knobs.go
1 change: 1 addition & 0 deletions pkg/kv/kvserver/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -198,6 +198,7 @@ go_library(
"//pkg/util/metric",
"//pkg/util/metric/aggmetric",
"//pkg/util/mon",
"//pkg/util/pprofutil",
"//pkg/util/protoutil",
"//pkg/util/quotapool",
"//pkg/util/retry",
17 changes: 16 additions & 1 deletion pkg/kv/kvserver/queue.go
Original file line number Diff line number Diff line change
@@ -14,6 +14,7 @@ import (
"container/heap"
"context"
"fmt"
"runtime/pprof"
"sync/atomic"
"time"

@@ -29,6 +30,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/util/hlc"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/metric"
"github.com/cockroachdb/cockroach/pkg/util/pprofutil"
"github.com/cockroachdb/cockroach/pkg/util/quotapool"
"github.com/cockroachdb/cockroach/pkg/util/stop"
"github.com/cockroachdb/cockroach/pkg/util/syncutil"
@@ -692,7 +694,17 @@ func (bq *baseQueue) maybeAdd(ctx context.Context, repl replicaInQueue, now hlc.
// it may not be and shouldQueue will be passed a nil realRepl. These tests
// know what they're getting into so that's fine.
realRepl, _ := repl.(*Replica)
should, priority := bq.impl.shouldQueue(ctx, now, realRepl, confReader)

labels := pprof.Labels("queue name", bq.name)
var should bool
var priority float64
pprof.Do(ctx, labels, func(ctx context.Context) {
if fn := bq.store.TestingKnobs().MaybeAddInterceptor; fn != nil {
fn(pprof.Label(ctx, "queue name"))
}
should, priority = bq.impl.shouldQueue(ctx, now, realRepl, confReader)
})

if !should {
return
}
@@ -824,6 +836,9 @@ func (bq *baseQueue) MaybeRemove(rangeID roachpb.RangeID) {
// stopper signals exit.
func (bq *baseQueue) processLoop(stopper *stop.Stopper) {
ctx := bq.AnnotateCtx(context.Background())
ctx, undo := pprofutil.SetProfilerLabelsFromCtxTags(ctx)
defer undo()

done := func() {
bq.mu.Lock()
bq.mu.stopped = true
37 changes: 37 additions & 0 deletions pkg/kv/kvserver/queue_test.go
Original file line number Diff line number Diff line change
@@ -14,6 +14,7 @@ import (
"container/heap"
"context"
"fmt"
"runtime/pprof"
"strconv"
"sync/atomic"
"testing"
@@ -561,6 +562,42 @@ func TestBaseQueueAddRemove(t *testing.T) {
}
}

// TestBaseQueueLabel verifies that queue name tag does not exist before and after maybeAdd was called but exist while maybeAdd is called.
func TestBaseQueueLabel(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
tc := testContext{}
stopper := stop.NewStopper()
ctx := context.Background()
defer stopper.Stop(ctx)
tc.Start(ctx, t, stopper)

r, err := tc.store.GetReplica(1)
if err != nil {
t.Fatal(err)
}

testQueue := &testQueueImpl{
shouldQueueFn: func(now hlc.ClockTimestamp, r *Replica) (shouldQueue bool, _ float64) {
shouldQueue = true
return
},
}
bq := makeTestBaseQueue("test", testQueue, tc.store, queueConfig{
acceptsUnsplitRanges: true,
})
bq.store.TestingKnobs().MaybeAddInterceptor = func(labelName string, labelDoesExist bool) {
require.True(t, labelDoesExist)
require.Equal(t, "test", labelName)
}
bq.Start(stopper)
bq.maybeAdd(ctx, r, hlc.ClockTimestamp{})

v, ok := pprof.Label(ctx, "queue name")
require.False(t, ok)
require.Equal(t, "", v)
}

// TestNeedsSystemConfig verifies that queues that don't need the system config
// are able to process replicas when the system config isn't available.
func TestNeedsSystemConfig(t *testing.T) {
3 changes: 3 additions & 0 deletions pkg/kv/kvserver/testing_knobs.go
Original file line number Diff line number Diff line change
@@ -465,6 +465,9 @@ type StoreTestingKnobs struct {

// RangeLeaseAcquireTimeoutOverride overrides RaftConfig.RangeLeaseAcquireTimeout().
RangeLeaseAcquireTimeoutOverride time.Duration

// MaybeAddInterceptor intercepts calls to MaybeAdd -> passes the function with the label
MaybeAddInterceptor func(labelName string, labelDoesExist bool)
}

// ModuleTestingKnobs is part of the base.ModuleTestingKnobs interface.