Skip to content

Commit 4dab0d8

Browse files
committed
kvserver: allow logs from callbacks up to 15 replicas per updateReplicationGauges
Previously, logs from the decommission nudger were not gated by a vmodule and could become spammy when many replicas were decommissioned at a low nudger frequency. This commit introduces a per-store budget, allowing logs from callbacks for up to 15 replicas per updateReplicationGauges call. Drawbacks of this approach: - Replicas are not visited in a sorted order, so we may be opening the floodgates from 15 different replicas each iteration. - Once a replica is permitted to log, its future logs from callbacks are not restricted. - If EnqueueProblemRangeInReplicateQueueInterval is set too low, 1 and 2 may become worse. For 1, we could consider visit the replica set with WithReplicasInOrder. I'm not sure about the overhead here since updateReplicationGauges is called periodically when collecting metrics. Here are the reasons that I think this approach is acceptable for now: - onEnqueueResult is unlikely to be reinvoked for replicas already in the queue unless they are processing or in purgatory (both are short-lived states we want visibility into). Once processed, replicas are removed from the set. onProcessResult should be called at most twice. For replicas merely waiting in the queue, the callback is not invoked, since their priority should not be actively updated. - We could cap logging per maybeEnqueueProblemRange, but granting full logging permission for each replica simplifies reasoning and gives complete visibility for specific replias. - In practice, escalations show that slow decommissioning usually involves <15 ranges, and EnqueueProblemRangeInReplicateQueueInterval is typically large (~15 minutes).
1 parent 9e0ba6b commit 4dab0d8

File tree

2 files changed

+43
-15
lines changed

2 files changed

+43
-15
lines changed

pkg/kv/kvserver/replica.go

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2918,8 +2918,9 @@ func (r *Replica) TestingRefreshLeaderlessWatcherUnavailableState(
29182918
// manner via the replica scanner, see #130199. This functionality is disabled
29192919
// by default for this reason.
29202920
func (r *Replica) maybeEnqueueProblemRange(
2921-
ctx context.Context, now time.Time, leaseValid, isLeaseholder bool,
2921+
ctx context.Context, now time.Time, leaseValid, isLeaseholder bool, shouldLog bool,
29222922
) {
2923+
29232924
// The method expects the caller to provide whether the lease is valid and
29242925
// the replica is the leaseholder for the range, so that it can avoid
29252926
// unnecessary work. We expect this method to be called in the context of
@@ -2961,24 +2962,41 @@ func (r *Replica) maybeEnqueueProblemRange(
29612962
r.store.replicateQueue.AddAsyncWithCallback(ctx, r,
29622963
allocatorimpl.AllocatorReplaceDecommissioningVoter.Priority(), processCallback{
29632964
onEnqueueResult: func(indexOnHeap int, err error) {
2964-
if err != nil {
2965-
// TODO(wenyihu6): if we want to put these logs behind vmodule, move
2966-
// this function to another file so that we can avoid the spam on
2967-
// other logs.
2968-
log.KvDistribution.Infof(ctx,
2969-
"decommissioning nudger failed to enqueue range %v due to %v", r.Desc(), err)
2965+
if shouldLog {
2966+
if err != nil {
2967+
log.KvDistribution.Infof(ctx,
2968+
"decommissioning nudger failed to enqueue range %v due to %v", r.Desc(), err)
2969+
} else {
2970+
log.KvDistribution.Infof(ctx,
2971+
"decommissioning nudger successfully enqueued range %v at index %d", r.Desc(), indexOnHeap)
2972+
}
29702973
} else {
2971-
log.KvDistribution.Infof(ctx,
2972-
"decommissioning nudger successfully enqueued range %v at index %d", r.Desc(), indexOnHeap)
2974+
if err != nil {
2975+
log.KvDistribution.VInfof(ctx, 2,
2976+
"decommissioning nudger failed to enqueue range %v due to %v", r.Desc(), err)
2977+
} else {
2978+
log.KvDistribution.VInfof(ctx, 2,
2979+
"decommissioning nudger successfully enqueued range %v at index %d", r.Desc(), indexOnHeap)
2980+
}
29732981
}
29742982
},
29752983
onProcessResult: func(err error) {
2976-
if err != nil {
2977-
log.KvDistribution.Infof(ctx,
2978-
"decommissioning nudger failed to process range %v due to %v", r.Desc(), err)
2984+
if shouldLog {
2985+
if err != nil {
2986+
log.KvDistribution.Infof(ctx,
2987+
"decommissioning nudger failed to process range %v due to %v", r.Desc(), err)
2988+
} else {
2989+
log.KvDistribution.Infof(ctx,
2990+
"decommissioning nudger successfully processed replica %s", r.Desc())
2991+
}
29792992
} else {
2980-
log.KvDistribution.Infof(ctx,
2981-
"decommissioning nudger successfully processed replica %s", r.Desc())
2993+
if err != nil {
2994+
log.KvDistribution.VInfof(ctx, 2,
2995+
"decommissioning nudger failed to process range %v due to %v", r.Desc(), err)
2996+
} else {
2997+
log.KvDistribution.VInfof(ctx, 2,
2998+
"decommissioning nudger successfully processed replica %s", r.Desc())
2999+
}
29823000
}
29833001
},
29843002
})

pkg/kv/kvserver/store.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3418,6 +3418,12 @@ func (s *Store) updateReplicationGauges(ctx context.Context) error {
34183418
ioOverload, _ = s.ioThreshold.t.Score()
34193419
s.ioThreshold.Unlock()
34203420

3421+
// TODO(wenyihu6): it would be nicer if we can sort the replicas so that we
3422+
// can always get the nudger story on the same set of replicas, will this
3423+
// introduce a lot of overhead? For now, it seems fine since we usually see <
3424+
// 15 ranges on decommission stall.
3425+
var logBudgetOnDecommissioningNudger = 15
3426+
34213427
// We want to avoid having to read this multiple times during the replica
34223428
// visiting, so load it once up front for all nodes.
34233429
livenessMap := s.cfg.NodeLiveness.ScanNodeVitalityFromCache()
@@ -3488,7 +3494,11 @@ func (s *Store) updateReplicationGauges(ctx context.Context) error {
34883494
if metrics.Decommissioning {
34893495
// NB: Enqueue is disabled by default from here and throttled async if
34903496
// enabled.
3491-
rep.maybeEnqueueProblemRange(ctx, goNow, metrics.LeaseValid, metrics.Leaseholder)
3497+
shouldLog := logBudgetOnDecommissioningNudger > 0
3498+
if shouldLog {
3499+
logBudgetOnDecommissioningNudger--
3500+
}
3501+
rep.maybeEnqueueProblemRange(ctx, goNow, metrics.LeaseValid, metrics.Leaseholder, shouldLog)
34923502
decommissioningRangeCount++
34933503
}
34943504
}

0 commit comments

Comments
 (0)