-
Notifications
You must be signed in to change notification settings - Fork 159
Pull based config changes from admin to runtime #8548
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
runtime/runtime.go
Outdated
| } | ||
|
|
||
| func (r *Runtime) ReloadConfig(ctx context.Context, instanceID string) error { | ||
| return r.configReloader.reloadConfig(ctx, instanceID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Need to check
if r.configReloader != nil { - Also, shouldn't it run in the background? Since the
adminservice may have to call this on many runtimes at a time, and since the runtime already is capable of pulling config in the background, it'd be nice for it to just be a quick op
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It will be nice to capture the error and forward it to admin. The status of the reloads can be checked in river jobs history as well. It can also reload if required.
Since the config reloads now happen async in admin service so it should be fine to introduce some latency.
But happy to make this change if you think otherwise.
begelundmuller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
closes https://linear.app/rilldata/issue/PLAT-324/switch-to-pull-instead-of-push-for-config-changes-from-admin-to
Checklist: