From 9fbf507f3eae0a37ad792288bd9878d1f31c6d93 Mon Sep 17 00:00:00 2001 From: arulajmani Date: Fri, 25 Feb 2022 17:00:52 -0500 Subject: [PATCH] spanconfigkvsubscriber,kvserver: fix KVSubscriber bug We had a bug in the KVSubscriber where we were invoking a copy of the handler instead of the handler stored. This meant that we'd never treat handlers as "initialized". As a result, we would always invoke them with the everything span, and as a result, visit all replicas on the stores in reaction to span config updates. See datadriven test diffs for an illustration. Fixing the above lead to unearthing an interesting bug in how we were deciding to enqueue replicas in the split queue. Previously, if we received a span config update that implied a split and the update corresponded to the right-hand side post split, we would skip enqueuing the replica in the split queue. The assumption was that we'd get an update corresponding to the LHS of the split for the same replica and that update would enqueue the replica. This doesn't always hold true though. For example, consider the case when a new table is created and must be split from its (left) adjacent table's range. This only results in a single update, corresponding to the new table's span, which is the right-hand side post split. This patch moves to nudging the split queue for all updates, not just left-hand side updates, for the reason above. Release note: None Release justification: bug fixes in new functionality --- pkg/kv/kvserver/store.go | 70 +++++++++---------- .../spanconfigkvsubscriber/kvsubscriber.go | 8 ++- .../spanconfigkvsubscriber/testdata/basic | 1 - .../testdata/buffer_overflow | 1 - .../testdata/system_span_configs | 2 - .../system_span_configs_secondary_tenants | 2 - 6 files changed, 39 insertions(+), 45 deletions(-) diff --git a/pkg/kv/kvserver/store.go b/pkg/kv/kvserver/store.go index cb2c980c64e7..a368853dd4e1 100644 --- a/pkg/kv/kvserver/store.go +++ b/pkg/kv/kvserver/store.go @@ -2395,45 +2395,43 @@ func (s *Store) onSpanConfigUpdate(ctx context.Context, updated roachpb.Span) { return nil // placeholder; ignore } + replCtx := repl.AnnotateCtx(ctx) startKey := repl.Desc().StartKey - if !sp.ContainsKey(startKey) { - // It's possible that the update we're receiving here is the - // right-hand side of a span config getting split. Think of - // installing a zone config on some partition of an index where - // previously there was none on any of the partitions. The range - // spanning the entire index would have to split on the - // partition boundary, and before it does so, it's possible that - // it would receive a span config update for just the partition. + if sp.ContainsKey(startKey) { + // It's possible that the update we're receiving here implies a split. + // If the update corresponds to what would be the config for the + // right-hand side after the split, we avoid clobbering the pre-split + // range's embedded span config by checking if the start key is part of + // the update. // - // To avoid clobbering the pre-split range's embedded span - // config with the partition's config, we'll ensure that the - // range's start key is part of the update. We don't have to - // enqueue the range in the split queue here, that takes place - // when processing the left-hand side span config update. - - return nil // ignore - } - - // TODO(irfansharif): It's possible for a config to be applied over an - // entire range when it only pertains to the first half of the range. - // This will be corrected shortly -- we enqueue the range for a split - // below where we then apply the right config on each half. But still, - // it's surprising behavior and gets in the way of a desirable - // consistency guarantee: a key's config at any point in time is one - // that was explicitly declared over it, or the default config. - // - // We can do better, we can skip applying the config entirely and - // enqueue the split, then relying on the split trigger to install - // the right configs on each half. The current structure is as it is - // to maintain parity with the system config span variant. - - replCtx := repl.AnnotateCtx(ctx) - conf, err := s.cfg.SpanConfigSubscriber.GetSpanConfigForKey(replCtx, startKey) - if err != nil { - log.Errorf(ctx, "skipped applying update, unexpected error reading from subscriber: %v", err) - return err + // Even if we're dealing with what would be the right-hand side after + // the split is processed, we still want to nudge the split queue + // below -- we can't instead rely on there being an update for the + // left-hand side of the split. Concretely, consider the case when a + // new table is added with a different configuration to its (left) + // adjacent table. This results in a single update, corresponding to the + // new table's span, which forms the right-hand side post split. + + // TODO(irfansharif): It's possible for a config to be applied over an + // entire range when it only pertains to the first half of the range. + // This will be corrected shortly -- we enqueue the range for a split + // below where we then apply the right config on each half. But still, + // it's surprising behavior and gets in the way of a desirable + // consistency guarantee: a key's config at any point in time is one + // that was explicitly declared over it, or the default config. + // + // We can do better, we can skip applying the config entirely and + // enqueue the split, then relying on the split trigger to install + // the right configs on each half. The current structure is as it is + // to maintain parity with the system config span variant. + + conf, err := s.cfg.SpanConfigSubscriber.GetSpanConfigForKey(replCtx, startKey) + if err != nil { + log.Errorf(ctx, "skipped applying update, unexpected error reading from subscriber: %v", err) + return err + } + repl.SetSpanConfig(conf) } - repl.SetSpanConfig(conf) // TODO(irfansharif): For symmetry with the system config span variant, // we queue blindly; we could instead only queue it if we knew the diff --git a/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go b/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go index 84ca69e3eaf7..0714c2bdbcec 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go +++ b/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go @@ -229,8 +229,9 @@ func (s *KVSubscriber) handleCompleteUpdate( s.mu.lastUpdated = ts handlers := s.mu.handlers s.mu.Unlock() - for _, h := range handlers { - h.invoke(ctx, keys.EverythingSpan) + for i := range handlers { + handler := &handlers[i] // mutated by invoke + handler.invoke(ctx, keys.EverythingSpan) } } @@ -248,7 +249,8 @@ func (s *KVSubscriber) handlePartialUpdate( handlers := s.mu.handlers s.mu.Unlock() - for _, handler := range handlers { + for i := range handlers { + handler := &handlers[i] // mutated by invoke for _, ev := range events { target := ev.(*bufferEvent).Update.Target handler.invoke(ctx, target.KeyspaceTargeted()) diff --git a/pkg/spanconfig/spanconfigkvsubscriber/testdata/basic b/pkg/spanconfig/spanconfigkvsubscriber/testdata/basic index d188e8dcaeed..54d052c09945 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/testdata/basic +++ b/pkg/spanconfig/spanconfigkvsubscriber/testdata/basic @@ -40,7 +40,6 @@ delete [d,f) updates ---- -[/Min,/Max) [d,f) store-reader key=d diff --git a/pkg/spanconfig/spanconfigkvsubscriber/testdata/buffer_overflow b/pkg/spanconfig/spanconfigkvsubscriber/testdata/buffer_overflow index 7a1b0d5397c4..997caa397365 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/testdata/buffer_overflow +++ b/pkg/spanconfig/spanconfigkvsubscriber/testdata/buffer_overflow @@ -75,7 +75,6 @@ upsert [a,c):C updates ---- -[/Min,/Max) [a,c) store-reader key=a diff --git a/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs b/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs index d345d8533e8d..200a7c790b76 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs +++ b/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs @@ -55,7 +55,6 @@ upsert {source=1, target=1}:Y # [/Min,/Tenant/2] corresponds to the system tenant targeting itself. updates ---- -[/Min,/Max) [/Min,/Tenant/2) # Ensure configs are correctly hydrated when we read them. @@ -93,7 +92,6 @@ upsert {source=1, target=1}:V updates ---- -[/Min,/Max) [/Min,/Tenant/2) store-reader key=a diff --git a/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs_secondary_tenants b/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs_secondary_tenants index 14fe1bfd8433..708e288b768d 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs_secondary_tenants +++ b/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs_secondary_tenants @@ -21,7 +21,6 @@ delete {source=1,target=20} updates ---- -[/Min,/Max) [/Tenant/20,/Tenant/21) # Lastly, update a system span config set on a secondary tenant's keyspace and @@ -32,5 +31,4 @@ upsert {source=10, target=10}:U updates ---- -[/Min,/Max) [/Tenant/10,/Tenant/11)