Skip to content

Commit

Permalink
Merge #95579
Browse files Browse the repository at this point in the history
95579: kvserver: fix store disable gossip lease override r=andrewbaptist a=kvoli

In #93945 the store gossip logic was refactored to take into account the relative difference rather than cumulative number of changes. This led to a bug where disabling lease capacity changes from triggering gossip wasn't adhered to in all cases.

This introduced a flake where `TestStoreRangeGossipOnSplits` and the underlying behavior being tested diverged slightly. This patch fixes the issue.

Fixes: #95566

Release note: None

Co-authored-by: Austen McClernon <[email protected]>
  • Loading branch information
craig[bot] and kvoli committed Jan 20, 2023
2 parents 875420d + 171458a commit 47d6f18
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 14 deletions.
18 changes: 12 additions & 6 deletions pkg/kv/kvserver/client_split_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2308,6 +2308,12 @@ func TestStoreRangeGossipOnSplits(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

// 50% for testing We can't properly test how frequently changes in the
// number of ranges trigger the store to gossip its capacities if we have
// to worry about changes in the number of leases also triggering store
// gossip.
overrideCapacityFraction := 0.5

ctx := context.Background()
serv, _, _ := serverutils.StartServer(t, base.TestServerArgs{
Knobs: base.TestingKnobs{
Expand All @@ -2316,11 +2322,8 @@ func TestStoreRangeGossipOnSplits(t *testing.T) {
DisableSplitQueue: true,
DisableScanner: true,
GossipTestingKnobs: kvserver.StoreGossipTestingKnobs{
OverrideGossipWhenCapacityDeltaExceedsFraction: 0.5, // 50% for testing
// We can't properly test how frequently changes in the number of ranges
// trigger the store to gossip its capacities if we have to worry about
// changes in the number of leases also triggering store gossip.
DisableLeaseCapacityGossip: true,
OverrideGossipWhenCapacityDeltaExceedsFraction: overrideCapacityFraction,
DisableLeaseCapacityGossip: true,
},
},
},
Expand Down Expand Up @@ -2385,7 +2388,10 @@ func TestStoreRangeGossipOnSplits(t *testing.T) {
}
select {
case rangeCount = <-rangeCountCh:
changeCount := int32(math.Ceil(math.Max(float64(lastRangeCount)*0.5, 10)))
changeCount := int32(math.Ceil(math.Max(
float64(lastRangeCount)*overrideCapacityFraction,
kvserver.GossipWhenRangeCountDeltaExceeds,
)))
diff := rangeCount - (lastRangeCount + changeCount)
if diff < -1 || diff > 1 {
t.Errorf("gossiped range count %d more than 1 away from expected %d", rangeCount, lastRangeCount+changeCount)
Expand Down
15 changes: 7 additions & 8 deletions pkg/kv/kvserver/store_gossip.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@ const (
// gossip interval.
gossipWhenLeaseCountDeltaExceeds = 5

// gossipWhenRangeCountDeltaExceeds specifies the absolute change from the
// GossipWhenRangeCountDeltaExceeds specifies the absolute change from the
// last gossiped store capacity range count which needs to be exceeded
// before the store will gossip immediately without waiting for the
// periodic gossip interval.
gossipWhenRangeCountDeltaExceeds = 5
GossipWhenRangeCountDeltaExceeds = 5

// gossipWhenLoadDeltaExceedsFraction specifies the fraction from the last
// gossiped store capacity load which needs to be exceeded before the store
Expand Down Expand Up @@ -394,9 +394,6 @@ const (
// immediate gossip of this store's descriptor, to include updated
// capacity information.
func (s *StoreGossip) MaybeGossipOnCapacityChange(ctx context.Context, cce CapacityChangeEvent) {
if s.knobs.DisableLeaseCapacityGossip && (cce == LeaseAddEvent || cce == LeaseRemoveEvent) {
return
}

// Incrementally adjust stats to keep them up to date even if the
// capacity is gossiped, but isn't due yet to be recomputed from scratch.
Expand Down Expand Up @@ -460,16 +457,18 @@ func (s *StoreGossip) shouldGossipOnCapacityDelta() (should bool, reason string)
updateForWPS, deltaWPS := deltaExceedsThreshold(
s.cachedCapacity.lastGossiped.WritesPerSecond, s.cachedCapacity.cached.WritesPerSecond,
gossipMinAbsoluteDelta, gossipWhenLoadDeltaExceedsFraction)

updateForRangeCount, deltaRangeCount := deltaExceedsThreshold(
float64(s.cachedCapacity.lastGossiped.RangeCount), float64(s.cachedCapacity.cached.RangeCount),
gossipWhenRangeCountDeltaExceeds, gossipWhenCapacityDeltaExceedsFraction)

GossipWhenRangeCountDeltaExceeds, gossipWhenCapacityDeltaExceedsFraction)
updateForLeaseCount, deltaLeaseCount := deltaExceedsThreshold(
float64(s.cachedCapacity.lastGossiped.LeaseCount), float64(s.cachedCapacity.cached.LeaseCount),
gossipWhenLeaseCountDeltaExceeds, gossipWhenCapacityDeltaExceedsFraction)
s.cachedCapacity.Unlock()

if s.knobs.DisableLeaseCapacityGossip {
updateForLeaseCount = false
}

if updateForQPS {
reason += fmt.Sprintf("queries-per-second(%.1f) ", deltaQPS)
}
Expand Down

0 comments on commit 47d6f18

Please sign in to comment.