Skip to content

Commit 37b40b8

Browse files
rafissrafiss
committed
systemconfigwatcher: remove NewWithAdditionalProvider
This was needed in the 21.2-22.1 mixed version state, before tenants had a spanconfig reconciliation job. Now that the job exists, there's no need to pass in an additional provider for tenants. Release note: None
1 parent 2e66bf7 commit 37b40b8

5 files changed

Lines changed: 10 additions & 264 deletions

File tree

‎pkg/server/systemconfigwatcher/BUILD.bazel‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,19 +23,14 @@ go_library(
2323
go_test(
2424
name = "systemconfigwatcher_test",
2525
srcs = [
26-
"cache_test.go",
2726
"main_test.go",
2827
"system_config_watcher_test.go",
2928
],
3029
deps = [
31-
":systemconfigwatcher",
3230
"//pkg/base",
3331
"//pkg/config",
34-
"//pkg/config/zonepb",
3532
"//pkg/keys",
3633
"//pkg/kv",
37-
"//pkg/kv/kvclient/kvtenant",
38-
"//pkg/kv/kvclient/rangefeed",
3934
"//pkg/kv/kvpb",
4035
"//pkg/roachpb",
4136
"//pkg/security/securityassets",
@@ -49,7 +44,6 @@ go_test(
4944
"//pkg/util/leaktest",
5045
"//pkg/util/log",
5146
"//pkg/util/protoutil",
52-
"//pkg/util/syncutil",
5347
"@com_github_cockroachdb_errors//:errors",
5448
"@com_github_kr_pretty//:pretty",
5549
"@com_github_stretchr_testify//assert",

‎pkg/server/systemconfigwatcher/cache.go‎

Lines changed: 5 additions & 107 deletions
Original file line numberDiff line numberDiff line change
@@ -26,52 +26,20 @@ import (
2626
// cache provides a consistent snapshot when available, but the snapshot
2727
// may be stale.
2828
type Cache struct {
29-
w *rangefeedcache.Watcher[*kvpb.RangeFeedValue]
30-
defaultZoneConfig *zonepb.ZoneConfig
31-
additionalKVsSource config.SystemConfigProvider
32-
mu struct {
29+
w *rangefeedcache.Watcher[*kvpb.RangeFeedValue]
30+
defaultZoneConfig *zonepb.ZoneConfig
31+
mu struct {
3332
syncutil.RWMutex
3433

3534
cfg *config.SystemConfig
3635

3736
registry notificationRegistry
38-
39-
// additionalKVs provides a mechanism for the creator of the
40-
// cache to provide additional values.
41-
//
42-
// This is used to support injecting some key-value pairs from the
43-
// system tenant into the system config.
44-
additionalKVs []roachpb.KeyValue
4537
}
4638
}
4739

4840
// New constructs a new Cache.
4941
func New(
5042
codec keys.SQLCodec, clock *hlc.Clock, f *rangefeed.Factory, defaultZoneConfig *zonepb.ZoneConfig,
51-
) *Cache {
52-
return NewWithAdditionalProvider(
53-
codec, clock, f, defaultZoneConfig, nil, /* additionalProvider */
54-
)
55-
}
56-
57-
// NewWithAdditionalProvider constructs a new Cache with the addition of
58-
// another provider of a SystemConfig. This additional provider is used only
59-
// for the KVs in its system config. The key-value pairs it provides should
60-
// not overlap with those of this provider, if they do, the latest values
61-
// will be preferred.
62-
//
63-
// This functionality exists to provide access to the system tenant's view
64-
// of its zone configuration for RANGE DEFAULT and RANGE TENANTS. This is
65-
// needed primarily in the mixed-version state before the tenant is in control
66-
// of its own zone configurations.
67-
//
68-
// TODO(ajwerner): Remove this functionality once it's no longer needed in 22.2.
69-
func NewWithAdditionalProvider(
70-
codec keys.SQLCodec,
71-
clock *hlc.Clock,
72-
f *rangefeed.Factory,
73-
defaultZoneConfig *zonepb.ZoneConfig,
74-
additional config.SystemConfigProvider,
7543
) *Cache {
7644
// TODO(ajwerner): Deal with what happens if the system config has more than this
7745
// many rows.
@@ -82,7 +50,6 @@ func NewWithAdditionalProvider(
8250
defaultZoneConfig: defaultZoneConfig,
8351
}
8452
c.mu.registry = notificationRegistry{}
85-
c.additionalKVsSource = additional
8653

8754
spans := []roachpb.Span{
8855
codec.TableSpan(keys.DescriptorTableID),
@@ -105,36 +72,6 @@ func (c *Cache) Start(ctx context.Context, stopper *stop.Stopper) error {
10572
if err := rangefeedcache.Start(ctx, stopper, c.w, nil /* onError */); err != nil {
10673
return err
10774
}
108-
if c.additionalKVsSource != nil {
109-
setAdditionalKeys := func() {
110-
if cfg := c.additionalKVsSource.GetSystemConfig(); cfg != nil {
111-
c.setAdditionalKeys(cfg.Values)
112-
}
113-
}
114-
ch, unregister := c.additionalKVsSource.RegisterSystemConfigChannel()
115-
// Check if there are any additional keys to set before returning from
116-
// start. This is mostly to make tests deterministic.
117-
select {
118-
case <-ch:
119-
setAdditionalKeys()
120-
default:
121-
}
122-
if err := stopper.RunAsyncTask(ctx, "systemconfigwatcher-additional", func(ctx context.Context) {
123-
for {
124-
select {
125-
case <-ctx.Done():
126-
return
127-
case <-stopper.ShouldQuiesce():
128-
return
129-
case <-ch:
130-
setAdditionalKeys()
131-
}
132-
}
133-
}); err != nil {
134-
unregister()
135-
return err
136-
}
137-
}
13875
return nil
13976
}
14077

@@ -163,46 +100,6 @@ func (c *Cache) RegisterSystemConfigChannel() (_ <-chan struct{}, unregister fun
163100
}
164101
}
165102

166-
func (c *Cache) setAdditionalKeys(kvs []roachpb.KeyValue) {
167-
c.mu.Lock()
168-
defer c.mu.Unlock()
169-
170-
sort.Sort(keyValues(kvs))
171-
if c.mu.cfg == nil {
172-
c.mu.additionalKVs = kvs
173-
return
174-
}
175-
176-
cloned := append([]roachpb.KeyValue(nil), c.mu.cfg.Values...)
177-
trimmed := append(trimOldKVs(cloned, c.mu.additionalKVs), kvs...)
178-
sort.Sort(keyValues(trimmed))
179-
c.mu.cfg = config.NewSystemConfig(c.defaultZoneConfig)
180-
c.mu.cfg.Values = trimmed
181-
c.mu.additionalKVs = kvs
182-
c.mu.registry.notify()
183-
}
184-
185-
// trimOldKVs removes KVs from cloned where for all keys in prev.
186-
// This function assumes that both cloned and prev are sorted.
187-
func trimOldKVs(cloned, prev []roachpb.KeyValue) []roachpb.KeyValue {
188-
trimmed := cloned[:0]
189-
shouldSkip := func(clonedOrd int) (shouldSkip bool) {
190-
for len(prev) > 0 {
191-
if cmp := prev[0].Key.Compare(cloned[clonedOrd].Key); cmp >= 0 {
192-
return cmp == 0
193-
}
194-
prev = prev[1:]
195-
}
196-
return false
197-
}
198-
for i := range cloned {
199-
if !shouldSkip(i) {
200-
trimmed = append(trimmed, cloned[i])
201-
}
202-
}
203-
return trimmed
204-
}
205-
206103
type keyValues []roachpb.KeyValue
207104

208105
func (k keyValues) Len() int { return len(k) }
@@ -221,7 +118,8 @@ func (c *Cache) handleUpdate(
221118
var updatedData []roachpb.KeyValue
222119
switch update.Type {
223120
case rangefeedcache.CompleteUpdate:
224-
updatedData = rangefeedbuffer.MergeKVs(c.mu.additionalKVs, updateKVs)
121+
sort.Sort(keyValues(updateKVs))
122+
updatedData = updateKVs
225123
case rangefeedcache.IncrementalUpdate:
226124
if len(updateKVs) == 0 {
227125
// Simply return since there is nothing interesting.

‎pkg/server/systemconfigwatcher/cache_test.go‎

Lines changed: 0 additions & 127 deletions
This file was deleted.

‎pkg/server/systemconfigwatcher/system_config_watcher_test.go‎

Lines changed: 4 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import (
1515
"github.com/cockroachdb/cockroach/pkg/config"
1616
"github.com/cockroachdb/cockroach/pkg/keys"
1717
"github.com/cockroachdb/cockroach/pkg/kv"
18-
"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvtenant"
1918
"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
2019
"github.com/cockroachdb/cockroach/pkg/roachpb"
2120
"github.com/cockroachdb/cockroach/pkg/sql"
@@ -38,7 +37,7 @@ func TestSystemConfigWatcher(t *testing.T) {
3837
defer log.Scope(t).Close(t)
3938

4039
ctx := context.Background()
41-
s, sqlDB, kvDB := serverutils.StartServer(t,
40+
s, sqlDB, _ := serverutils.StartServer(t,
4241
base.TestServerArgs{
4342
DefaultTestTenant: base.TestControlsTenantsExplicitly,
4443
},
@@ -52,30 +51,17 @@ func TestSystemConfigWatcher(t *testing.T) {
5251
tdb.Exec(t, "SET CLUSTER SETTING kv.rangefeed.closed_timestamp_refresh_interval = '10 ms'")
5352

5453
t.Run("system", func(t *testing.T) {
55-
runTest(t, s, sqlDB, nil)
54+
runTest(t, s, sqlDB)
5655
})
5756
t.Run("secondary", func(t *testing.T) {
5857
tenant, tenantDB := serverutils.StartTenant(t, s, base.TestTenantArgs{
5958
TenantID: serverutils.TestTenantID(),
6059
})
61-
// We expect the secondary tenant to see the host tenant's view of a few
62-
// keys. We need to plumb that expectation into the test.
63-
runTest(t, tenant, tenantDB, func(t *testing.T) []roachpb.KeyValue {
64-
return kvtenant.GossipSubscriptionSystemConfigMask.Apply(
65-
config.SystemConfigEntries{
66-
Values: getSystemDescriptorAndZonesSpans(ctx, t, keys.SystemSQLCodec, kvDB),
67-
},
68-
).Values
69-
})
60+
runTest(t, tenant, tenantDB)
7061
})
7162
}
7263

73-
func runTest(
74-
t *testing.T,
75-
s serverutils.ApplicationLayerInterface,
76-
sqlDB *gosql.DB,
77-
extraRows func(t *testing.T) []roachpb.KeyValue,
78-
) {
64+
func runTest(t *testing.T, s serverutils.ApplicationLayerInterface, sqlDB *gosql.DB) {
7965
ctx := context.Background()
8066
tdb := sqlutils.MakeSQLRunner(sqlDB)
8167
execCfg := s.ExecutorConfig().(sql.ExecutorConfig)
@@ -95,10 +81,6 @@ func runTest(
9581
}
9682
entries := protoutil.Clone(&rs.SystemConfigEntries).(*config.SystemConfigEntries)
9783
sc := getSystemDescriptorAndZonesSpans(ctx, t, execCfg.Codec, kvDB)
98-
if extraRows != nil {
99-
sc = append(sc, extraRows(t)...)
100-
sort.Sort(roachpb.KeyValueByKey(sc))
101-
}
10284
sort.Sort(roachpb.KeyValueByKey(entries.Values))
10385
if !assert.Equal(noopT{}, sc, entries.Values) {
10486
return errors.Errorf("mismatch: %v", pretty.Diff(sc, entries.Values))

‎pkg/server/tenant.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1248,9 +1248,8 @@ func makeTenantSQLServerArgs(
12481248

12491249
sTS := ts.MakeTenantServer(baseCfg.AmbientCtx, tenantConnect, rpcContext.TenantID, registry)
12501250

1251-
systemConfigWatcher := systemconfigwatcher.NewWithAdditionalProvider(
1251+
systemConfigWatcher := systemconfigwatcher.New(
12521252
keys.MakeSQLCodec(sqlCfg.TenantID), clock, rangeFeedFactory, &baseCfg.DefaultZoneConfig,
1253-
tenantConnect,
12541253
)
12551254

12561255
// Define structures which have circular dependencies. The underlying structures

0 commit comments

Comments
 (0)