From e3792a5d83a2986020f239c4ab4d0eac94987ee5 Mon Sep 17 00:00:00 2001 From: irfan sharif Date: Fri, 10 Dec 2021 16:46:35 -0500 Subject: [PATCH] spanconfig: improve typing for Update By using a type definition instead of a stand-alone type, we can reduce the amount of code needed to convert from a SpanConfigEntry to an Update (something we frequently do). It also helps make it less error prone. While here, we introduce two helpful constructors for the two kinds of updates we're typically interested in -- additions and deletions. Release note: None --- pkg/kv/kvserver/client_spanconfigs_test.go | 2 +- pkg/spanconfig/spanconfig.go | 30 +++++++++++----- .../spanconfigkvsubscriber/kvsubscriber.go | 8 +++-- .../spanconfigreconciler/reconciler.go | 34 +++++-------------- pkg/spanconfig/spanconfigstore/store.go | 5 +-- pkg/spanconfig/spanconfigstore/store_test.go | 19 +++-------- .../spanconfigtestutils/recorder.go | 4 +-- pkg/spanconfig/spanconfigtestutils/utils.go | 4 +-- 8 files changed, 45 insertions(+), 61 deletions(-) diff --git a/pkg/kv/kvserver/client_spanconfigs_test.go b/pkg/kv/kvserver/client_spanconfigs_test.go index 3995e4748eaf..fcc9354ee9b2 100644 --- a/pkg/kv/kvserver/client_spanconfigs_test.go +++ b/pkg/kv/kvserver/client_spanconfigs_test.go @@ -69,7 +69,7 @@ func TestSpanConfigUpdateAppliedToReplica(t *testing.T) { span := repl.Desc().RSpan().AsRawSpanWithNoLocals() conf := roachpb.SpanConfig{NumReplicas: 5, NumVoters: 3} - deleted, added := spanConfigStore.Apply(ctx, false /* dryrun */, spanconfig.Update{Span: span, Config: conf}) + deleted, added := spanConfigStore.Apply(ctx, false /* dryrun */, spanconfig.Addition(span, conf)) require.Empty(t, deleted) require.Len(t, added, 1) require.True(t, added[0].Span.Equal(span)) diff --git a/pkg/spanconfig/spanconfig.go b/pkg/spanconfig/spanconfig.go index c50b455d9c7f..118035b4b498 100644 --- a/pkg/spanconfig/spanconfig.go +++ b/pkg/spanconfig/spanconfig.go @@ -232,8 +232,8 @@ type StoreReader interface { GetSpanConfigForKey(ctx context.Context, key roachpb.RKey) (roachpb.SpanConfig, error) } -// DescriptorUpdate captures the ID and type of a descriptor or zone that the -// SQLWatcher has observed updated. +// DescriptorUpdate captures the ID and the type of descriptor or zone that been +// updated. It's the unit of what the SQLWatcher emits. type DescriptorUpdate struct { // ID of the descriptor/zone that has been updated. ID descpb.ID @@ -244,14 +244,26 @@ type DescriptorUpdate struct { } // Update captures a span and the corresponding config change. It's the unit of -// what can be applied to a StoreWriter. -type Update struct { - // Span captures the key span being updated. - Span roachpb.Span +// what can be applied to a StoreWriter. The embedded span captures what's being +// updated; the config captures what it's being updated to. An empty config +// indicates a deletion. +type Update roachpb.SpanConfigEntry - // Config captures the span config the key span was updated to. An empty - // config indicates the span config being deleted. - Config roachpb.SpanConfig +// Deletion constructs an update that represents a deletion over the given span. +func Deletion(span roachpb.Span) Update { + return Update{ + Span: span, + Config: roachpb.SpanConfig{}, // delete + } +} + +// Addition constructs an update that represents adding the given config over +// the given span. +func Addition(span roachpb.Span, conf roachpb.SpanConfig) Update { + return Update{ + Span: span, + Config: conf, + } } // Deletion returns true if the update corresponds to a span config being diff --git a/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go b/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go index 84d7746626cb..3f738c2ee94a 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go +++ b/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go @@ -255,9 +255,11 @@ func (s *KVSubscriber) run(ctx context.Context) error { log.Infof(ctx, "received span configuration update for %s (deleted=%t)", entry.Span, deleted) } - update := spanconfig.Update{Span: entry.Span} - if !deleted { - update.Config = entry.Config + var update spanconfig.Update + if deleted { + update = spanconfig.Deletion(entry.Span) + } else { + update = spanconfig.Update(entry) } if err := buffer.Add(&bufferEvent{update, ev.Value.Timestamp}); err != nil { diff --git a/pkg/spanconfig/spanconfigreconciler/reconciler.go b/pkg/spanconfig/spanconfigreconciler/reconciler.go index d6e0e5743eac..01e9e9325091 100644 --- a/pkg/spanconfig/spanconfigreconciler/reconciler.go +++ b/pkg/spanconfig/spanconfigreconciler/reconciler.go @@ -187,10 +187,7 @@ func (f *fullReconciler) reconcile( updates := make([]spanconfig.Update, len(entries)) for i, entry := range entries { - updates[i] = spanconfig.Update{ - Span: entry.Span, - Config: entry.Config, - } + updates[i] = spanconfig.Update(entry) } toDelete, toUpsert := storeWithExistingSpanConfigs.Apply(ctx, false /* dryrun */, updates...) @@ -215,11 +212,8 @@ func (f *fullReconciler) reconcile( // get rid of them below. var storeWithExtraneousSpanConfigs *spanconfigstore.Store { - for _, update := range updates { - storeWithExistingSpanConfigs.Apply(ctx, false /* dryrun */, spanconfig.Update{ - Span: update.Span, - Config: roachpb.SpanConfig{}, // delete - }) + for _, u := range updates { + storeWithExistingSpanConfigs.Apply(ctx, false /* dryrun */, spanconfig.Deletion(u.Span)) } storeWithExtraneousSpanConfigs = storeWithExistingSpanConfigs } @@ -232,11 +226,8 @@ func (f *fullReconciler) reconcile( // Update the store that's supposed to reflect the latest span config // contents. As before, we could've fetched this state from KV directly, but // doing it this way is cheaper. - for _, deletedSpan := range deletedSpans { - storeWithLatestSpanConfigs.Apply(ctx, false /* dryrun */, spanconfig.Update{ - Span: deletedSpan, - Config: roachpb.SpanConfig{}, // delete - }) + for _, d := range deletedSpans { + storeWithLatestSpanConfigs.Apply(ctx, false /* dryrun */, spanconfig.Deletion(d)) } return storeWithLatestSpanConfigs, reconciledUpUntil, nil @@ -279,10 +270,7 @@ func (f *fullReconciler) fetchExistingSpanConfigs( } for _, entry := range entries { - store.Apply(ctx, false /* dryrun */, spanconfig.Update{ - Span: entry.Span, - Config: entry.Config, - }) + store.Apply(ctx, false /* dryrun */, spanconfig.Update(entry)) } } return store, nil @@ -361,10 +349,7 @@ func (r *incrementalReconciler) reconcile( updates := make([]spanconfig.Update, 0, len(missingTableIDs)+len(entries)) for _, entry := range entries { // Update span configs for SQL state that changed. - updates = append(updates, spanconfig.Update{ - Span: entry.Span, - Config: entry.Config, - }) + updates = append(updates, spanconfig.Update(entry)) } for _, missingID := range missingTableIDs { // Delete span configs for missing tables. @@ -372,10 +357,7 @@ func (r *incrementalReconciler) reconcile( Key: r.codec.TablePrefix(uint32(missingID)), EndKey: r.codec.TablePrefix(uint32(missingID)).PrefixEnd(), } - updates = append(updates, spanconfig.Update{ - Span: tableSpan, - Config: roachpb.SpanConfig{}, // delete - }) + updates = append(updates, spanconfig.Deletion(tableSpan)) } toDelete, toUpsert := r.storeWithKVContents.Apply(ctx, false /* dryrun */, updates...) diff --git a/pkg/spanconfig/spanconfigstore/store.go b/pkg/spanconfig/spanconfigstore/store.go index 19dde8e9fe42..8a226ae5fc50 100644 --- a/pkg/spanconfig/spanconfigstore/store.go +++ b/pkg/spanconfig/spanconfigstore/store.go @@ -146,10 +146,7 @@ func (s *Store) Apply( func (s *Store) Copy(ctx context.Context) *Store { clone := New(s.fallback) _ = s.ForEachOverlapping(ctx, keys.EverythingSpan, func(entry roachpb.SpanConfigEntry) error { - clone.Apply(ctx, false /* dryrun */, spanconfig.Update{ - Span: entry.Span, - Config: entry.Config, - }) + clone.Apply(ctx, false /* dryrun */, spanconfig.Update(entry)) return nil }) return clone diff --git a/pkg/spanconfig/spanconfigstore/store_test.go b/pkg/spanconfig/spanconfigstore/store_test.go index 1cde8d103d16..00bb9063a6c2 100644 --- a/pkg/spanconfig/spanconfigstore/store_test.go +++ b/pkg/spanconfig/spanconfigstore/store_test.go @@ -181,9 +181,9 @@ func TestRandomized(t *testing.T) { sp, conf, op := genRandomSpan(), getRandomConf(), getRandomOp() switch op { case "set": - return spanconfig.Update{Span: sp, Config: conf} + return spanconfig.Addition(sp, conf) case "del": - return spanconfig.Update{Span: sp} + return spanconfig.Deletion(sp) default: } t.Fatalf("unexpected op: %s", op) @@ -318,18 +318,9 @@ func TestStoreClone(t *testing.T) { everything := spanconfigtestutils.ParseSpan(t, "[a, z)") updates := []spanconfig.Update{ - { - Span: spanconfigtestutils.ParseSpan(t, "[a, b)"), - Config: spanconfigtestutils.ParseConfig(t, "A"), - }, - { - Span: spanconfigtestutils.ParseSpan(t, "[c, d)"), - Config: spanconfigtestutils.ParseConfig(t, "C"), - }, - { - Span: spanconfigtestutils.ParseSpan(t, "[e, f)"), - Config: spanconfigtestutils.ParseConfig(t, "E"), - }, + spanconfig.Addition(spanconfigtestutils.ParseSpan(t, "[a, b)"), spanconfigtestutils.ParseConfig(t, "A")), + spanconfig.Addition(spanconfigtestutils.ParseSpan(t, "[c, d)"), spanconfigtestutils.ParseConfig(t, "C")), + spanconfig.Addition(spanconfigtestutils.ParseSpan(t, "[e, f)"), spanconfigtestutils.ParseConfig(t, "E")), } original := New(roachpb.TestingDefaultSpanConfig()) diff --git a/pkg/spanconfig/spanconfigtestutils/recorder.go b/pkg/spanconfig/spanconfigtestutils/recorder.go index 7782badbed33..0db4cf8db685 100644 --- a/pkg/spanconfig/spanconfigtestutils/recorder.go +++ b/pkg/spanconfig/spanconfigtestutils/recorder.go @@ -68,13 +68,13 @@ func (r *KVAccessorRecorder) UpdateSpanConfigEntries( for _, d := range toDelete { r.mu.mutations = append(r.mu.mutations, mutation{ - update: spanconfig.Update{Span: d}, + update: spanconfig.Deletion(d), batchIdx: r.mu.batchCount, }) } for _, u := range toUpsert { r.mu.mutations = append(r.mu.mutations, mutation{ - update: spanconfig.Update{Span: u.Span, Config: u.Config}, + update: spanconfig.Update(u), batchIdx: r.mu.batchCount, }) } diff --git a/pkg/spanconfig/spanconfigtestutils/utils.go b/pkg/spanconfig/spanconfigtestutils/utils.go index 7b057b6a77cb..04a87ea4a335 100644 --- a/pkg/spanconfig/spanconfigtestutils/utils.go +++ b/pkg/spanconfig/spanconfigtestutils/utils.go @@ -157,11 +157,11 @@ func ParseStoreApplyArguments(t *testing.T, input string) (updates []spanconfig. switch { case strings.HasPrefix(line, deletePrefix): line = strings.TrimPrefix(line, line[:len(deletePrefix)]) - updates = append(updates, spanconfig.Update{Span: ParseSpan(t, line)}) + updates = append(updates, spanconfig.Deletion(ParseSpan(t, line))) case strings.HasPrefix(line, setPrefix): line = strings.TrimPrefix(line, line[:len(setPrefix)]) entry := ParseSpanConfigEntry(t, line) - updates = append(updates, spanconfig.Update{Span: entry.Span, Config: entry.Config}) + updates = append(updates, spanconfig.Update(entry)) default: t.Fatalf("malformed line %q, expected to find prefix %q or %q", line, setPrefix, deletePrefix)