Skip to content

Commit

Permalink
kvserver: disable sendWithRangeID call stack
Browse files Browse the repository at this point in the history
Sadly, this on longer embeds the RangeID in the stack trace (likely
culprit: Go adopting a register-based calling convention) Instead, you
get garbage values that often are obviously garbage, but this may not
always be true. We want to avoid being misled, so for now remove the
rangeID here and explain when it can come back.

See: https://cockroachlabs.slack.com/archives/G01G8LK77DK/p1641478596004700

Release note: None
  • Loading branch information
tbg committed Jan 20, 2022
1 parent e8a0b75 commit bd72f75
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 4 deletions.
15 changes: 12 additions & 3 deletions pkg/kv/kvserver/replica_send.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ var optimisticEvalLimitedScans = settings.RegisterBoolSetting(
func (r *Replica) Send(
ctx context.Context, ba roachpb.BatchRequest,
) (*roachpb.BatchResponse, *roachpb.Error) {
return r.sendWithRangeID(ctx, r.RangeID, &ba)
return r.sendWithoutRangeID(ctx, &ba)
}

// checkCircuitBreaker takes a cancelable context and its cancel function. The
Expand Down Expand Up @@ -163,6 +163,15 @@ func maybeAdjustWithBreakerError(pErr *roachpb.Error, brErr error) *roachpb.Erro
return pErr
}

// sendWithoutRangeID used to be called sendWithRangeID, accepted a `_forStacks
// roachpb.RangeID` argument, and had the description below. Ever since Go
// switched to the register-based calling convention though, this stopped
// working, giving essentially random numbers in the goroutine dumps that were
// misleading. It has thus been "disarmed" until Go produces useful values
// again.
//
// See (internal): https://cockroachlabs.slack.com/archives/G01G8LK77DK/p1641478596004700
//
// sendWithRangeID takes an unused rangeID argument so that the range
// ID will be accessible in stack traces (both in panics and when
// sampling goroutines from a live server). This line is subject to
Expand All @@ -174,8 +183,8 @@ func maybeAdjustWithBreakerError(pErr *roachpb.Error, brErr error) *roachpb.Erro
// within the portion printed in the stack trace):
//
// github.com/cockroachdb/cockroach/pkg/storage.(*Replica).sendWithRangeID(0xc420d1a000, 0x64bfb80, 0xc421564b10, 0x15, 0x153fd4634aeb0193, 0x0, 0x100000001, 0x1, 0x15, 0x0, ...)
func (r *Replica) sendWithRangeID(
ctx context.Context, _forStacks roachpb.RangeID, ba *roachpb.BatchRequest,
func (r *Replica) sendWithoutRangeID(
ctx context.Context, ba *roachpb.BatchRequest,
) (_ *roachpb.BatchResponse, rErr *roachpb.Error) {
var br *roachpb.BatchResponse
if r.leaseholderStats != nil && ba.Header.GatewayNodeID != 0 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/replica_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12667,7 +12667,7 @@ func TestProposalNotAcknowledgedOrReproposedAfterApplication(t *testing.T) {

// Round trip another proposal through the replica to ensure that previously
// committed entries have been applied.
_, pErr = tc.repl.sendWithRangeID(ctx, tc.repl.RangeID, &ba)
_, pErr = tc.repl.sendWithoutRangeID(ctx, &ba)
if pErr != nil {
t.Fatal(pErr)
}
Expand Down

0 comments on commit bd72f75

Please sign in to comment.