Gradual config rollouts #8922
Gradual config rollouts #8922
Conversation
ShahabT
left a comment
ShahabT
left a comment
There was a problem hiding this comment.
nice feature! I did not review the implementation. but the concept lgtm
| fairnessChangeChanged := makeChangeChangedFunc(changeKey, &fairness, unload) | ||
| fairnessChange, cancel1 := tqConfig.EnableFairnessSub(fairnessChangeChanged) | ||
| fairness = fairnessChange.Value(changeKey, time.Now()) | ||
| pm.cancelFairnessSub = cancel1 |
There was a problem hiding this comment.
I think it makes sense to extract this into a helper that can enable you saying the same thing as before, but giving it the changeKey too:
fairness, pm.cancelFairnessSub = tqConfig.EnableFairnessSub(changeKey, unload)
Doesn't have to be in this PR though.
There was a problem hiding this comment.
Yup, I definitely meant this as a "draft".. the api/helpers can be improved for sure
|
|
||
| // ConstantGradualChange returns a GradualChange whose Value always returns def and whose When | ||
| // always returns a time in the past. | ||
| func ConstantGradualChange[T any](def T) GradualChange[T] { |
There was a problem hiding this comment.
Small nit: This took me a second to realize that this was the default "non changing" variant, perhaps something like StaticGradualChange() or FixedGradualChange()?
There was a problem hiding this comment.
Right, "Constant" was supposed to be that :) But "constant change" is misleading. Static seems a little better, yeah
|
|
||
| var newTmr clock.Timer | ||
| if at := w.change.When(w.changeKey); at.After(now) { | ||
| newTmr = w.clock.AfterFunc(at.Sub(now), w.timerCallback) |
There was a problem hiding this comment.
Is it possible to hit some pretty deep recursion here? considering that the timerCallback calls back into reevalLocked. Not too familiar with the clock code here.
There was a problem hiding this comment.
It's just timer.AfterFunc, but fake-able. When the timer callback fires, it should re-evaluate and see that the change is in the past, so it doesn't need another timer. With some very contrived sequence of wall clock jumps it could end up with multiple callbacks in a row, but it should sort itself out quickly as long as time is mostly monotonic.
| tqConfig.NewMatcher = true | ||
| } else { | ||
| tqConfig.NewMatcher, pm.cancelNewMatcherSub = tqConfig.NewMatcherSub(unload) | ||
| tqConfig.NewMatcher, pm.cancelNewMatcherSub = dynamicconfig.SubscribeGradualChange( |
There was a problem hiding this comment.
Will we need to know if New value was returned for a namespace at a particular time when debugging an issue? Should we consider writing a log line when a value changes for a changeKey?
There was a problem hiding this comment.
For this particular purpose, we'll see logs for unloading/reloading task queues. So I'm not sure we need another here. Plus getting a logger into the subscription stuff is annoying.
| // key in a GradualChange). | ||
| if vmap, ok := v.(map[string]any); ok { | ||
| for k := range vmap { | ||
| if strings.ToLower(k) == "new" { |
There was a problem hiding this comment.
Isn't this a bit fragile? What if T is another struct with a field new?
We can also use a marker field that is unlikely to collide with another field
Gradual bool `mapstructure:"_gradual_"`
There was a problem hiding this comment.
Yeah.. I had a similar idea but I wanted to get this in sooner and then improve it later, also with feedback from other teams. The initial uses are only bools so it doesn't matter. We could even disable its use for structs for now.
4 participants
What changed?
Support gradual config rollouts by key (e.g. task queue partition).
Why?
Rolling out changes slowly, and also mostly-synchronized across processes.
How did you test it?
Potential risks
Time-based changes can be risky if you set something for the future and forget about it. We should have validation that prevents changes from being set too far in the future or outside of business hours.