settings: move the 'modified' map to valueContainer

knz · knz · commit 2f5d717178a8 · 2023-09-25T15:22:58.000+02:00
In a later commit, we will want to ensure that `.Override()` sets the
modified bit. This is not possible if the modified bits are in the
Updater. (The `Override` method is on the setting itself and has no
access to the updater.)

Release note: None
diff --git a/pkg/settings/settings_test.go b/pkg/settings/settings_test.go
@@ -574,23 +574,19 @@ func TestCache(t *testing.T) {
 		if expected, actual := true, boolFA.Get(sv); expected != actual {
 			t.Fatalf("expected %v, got %v", expected, actual)
 		}
-		// If the updater doesn't have a key, e.g. if the setting has been deleted,
-		// Resetting it from the cache.
+		// The updated status remains in sv. A new updater is able to pick
+		// it up.
 		settings.NewUpdater(sv).ResetRemaining(ctx)
 
-		if expected, actual := 2, changes.boolTA; expected != actual {
+		if expected, actual := 1, changes.boolTA; expected != actual {
 			t.Fatalf("expected %d, got %d", expected, actual)
 		}
 
-		if expected, actual := 2, changes.i1A; expected != actual {
+		if expected, actual := 1, changes.i1A; expected != actual {
 			t.Fatalf("expected %d, got %d", expected, actual)
 		}
 
-		if expected, actual := false, boolFA.Get(sv); expected != actual {
-			t.Fatalf("expected %v, got %v", expected, actual)
-		}
-
-		if expected, actual := false, boolFA.Get(sv); expected != actual {
+		if expected, actual := true, boolFA.Get(sv); expected != actual {
 			t.Fatalf("expected %v, got %v", expected, actual)
 		}
 	})
diff --git a/pkg/settings/updater.go b/pkg/settings/updater.go
@@ -60,7 +60,6 @@ func EncodeProtobuf(p protoutil.Message) string {
 
 type updater struct {
 	sv *Values
-	m  map[InternalKey]struct{}
 }
 
 // Updater is a helper for updating the in-memory settings.
@@ -92,7 +91,6 @@ func NewUpdater(sv *Values) Updater {
 		return NoopUpdater{}
 	}
 	return updater{
-		m:  make(map[InternalKey]struct{}, len(registry)),
 		sv: sv,
 	}
 }
@@ -108,7 +106,7 @@ func (u updater) Set(ctx context.Context, key InternalKey, value EncodedValue) e
 		return errors.Errorf("unknown setting '%s'", key)
 	}
 
-	u.m[key] = struct{}{}
+	u.sv.container.modified[d.getSlot()] = true
 
 	if expected := d.Typ(); value.Type != expected {
 		return errors.Errorf("setting '%s' defined as type %s, not %s", d.Name(), expected, value.Type)
@@ -167,19 +165,19 @@ func (u updater) Set(ctx context.Context, key InternalKey, value EncodedValue) e
 
 // ResetRemaining sets all settings not updated by the updater to their default values.
 func (u updater) ResetRemaining(ctx context.Context) {
-	for k, v := range registry {
-
-		if _, hasOverride := u.m[k]; hasOverride {
-			u.sv.setValueOrigin(ctx, v.getSlot(), OriginExplicitlySet)
+	for _, v := range registry {
+		slot := v.getSlot()
+		if hasOverride := u.sv.container.modified[slot]; hasOverride {
+			u.sv.setValueOrigin(ctx, slot, OriginExplicitlySet)
 		} else {
-			u.sv.setValueOrigin(ctx, v.getSlot(), OriginDefault)
+			u.sv.setValueOrigin(ctx, slot, OriginDefault)
 		}
 
 		if u.sv.SpecializedToVirtualCluster() && v.Class() == SystemOnly {
 			// Don't try to reset system settings on a non-system tenant.
 			continue
 		}
-		if _, ok := u.m[k]; !ok {
+		if m := u.sv.container.modified[slot]; !m {
 			v.setToDefault(ctx, u.sv)
 		}
 	}
diff --git a/pkg/settings/values.go b/pkg/settings/values.go
@@ -87,6 +87,11 @@ type valuesContainer struct {
 	forbidden [numSlots]bool
 
 	hasValue [numSlots]uint32
+
+	// modified is set when a setting is explictly set via Set()
+	// in the updater.
+	// TODO(knz): Check if this can be merged with hasValue above.
+	modified [numSlots]bool
 }
 
 func (c *valuesContainer) setGenericVal(slot slotIdx, newVal interface{}) {

Original file line number	Diff line number	Diff line change
`@@ -60,7 +60,6 @@ func EncodeProtobuf(p protoutil.Message) string {`
`60`	`60`
`61`	`61`	`type updater struct {`
`62`	`62`	`sv *Values`
`63`		`- m map[InternalKey]struct{}`
`64`	`63`	`}`
`65`	`64`
`66`	`65`	`// Updater is a helper for updating the in-memory settings.`
`@@ -92,7 +91,6 @@ func NewUpdater(sv *Values) Updater {`
`92`	`91`	`return NoopUpdater{}`
`93`	`92`	`}`
`94`	`93`	`return updater{`
`95`		`- m: make(map[InternalKey]struct{}, len(registry)),`
`96`	`94`	`sv: sv,`
`97`	`95`	`}`
`98`	`96`	`}`
`@@ -108,7 +106,7 @@ func (u updater) Set(ctx context.Context, key InternalKey, value EncodedValue) e`
`108`	`106`	`return errors.Errorf("unknown setting '%s'", key)`
`109`	`107`	`}`
`110`	`108`
`111`		`- u.m[key] = struct{}{}`
	`109`	`+ u.sv.container.modified[d.getSlot()] = true`
`112`	`110`
`113`	`111`	`if expected := d.Typ(); value.Type != expected {`
`114`	`112`	`return errors.Errorf("setting '%s' defined as type %s, not %s", d.Name(), expected, value.Type)`
`@@ -167,19 +165,19 @@ func (u updater) Set(ctx context.Context, key InternalKey, value EncodedValue) e`
`167`	`165`
`168`	`166`	`// ResetRemaining sets all settings not updated by the updater to their default values.`
`169`	`167`	`func (u updater) ResetRemaining(ctx context.Context) {`
`170`		`- for k, v := range registry {`
`171`		`-`
`172`		`- if _, hasOverride := u.m[k]; hasOverride {`
`173`		`- u.sv.setValueOrigin(ctx, v.getSlot(), OriginExplicitlySet)`
	`168`	`+ for _, v := range registry {`
	`169`	`+ slot := v.getSlot()`
	`170`	`+ if hasOverride := u.sv.container.modified[slot]; hasOverride {`
	`171`	`+ u.sv.setValueOrigin(ctx, slot, OriginExplicitlySet)`
`174`	`172`	`} else {`
`175`		`- u.sv.setValueOrigin(ctx, v.getSlot(), OriginDefault)`
	`173`	`+ u.sv.setValueOrigin(ctx, slot, OriginDefault)`
`176`	`174`	`}`
`177`	`175`
`178`	`176`	`if u.sv.SpecializedToVirtualCluster() && v.Class() == SystemOnly {`
`179`	`177`	`// Don't try to reset system settings on a non-system tenant.`
`180`	`178`	`continue`
`181`	`179`	`}`
`182`		`- if _, ok := u.m[k]; !ok {`
	`180`	`+ if m := u.sv.container.modified[slot]; !m {`
`183`	`181`	`v.setToDefault(ctx, u.sv)`
`184`	`182`	`}`
`185`	`183`	`}`