feat(config): Prepare configuration to be hot reloadable - #6346
feat(config): Prepare configuration to be hot reloadable #6346Dav1dde wants to merge 11 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
loewenheim
left a comment
loewenheim
left a comment
There was a problem hiding this comment.
LGTM in the sense that I'm sure it doesn't change any current behavior, but I confess I haven't got a very good mental model of when/where a config snapshot should be taken and where it should be passed down to a subsequent function. In other words, where's the boundary from which a config should be immutable?
| /// The snapshot should not be stored in a long lasting datastructure. As a rule of thumb it should | ||
| /// only exist on the stack. |
There was a problem hiding this comment.
IMO the name "snapshot" clearly communicates that it's a frozen copy of volatile data, so shouldn't we allow the user of the type decide how long they can keep the snapshot around? In other words, make inner an Arc<ConfigInner>?
There was a problem hiding this comment.
I was thinking of making a second variant/version of this which does own an Arc<>. I chose this variant with the comment because this is already what we're doing in Relay and a load() is more performant than a load_full(). If someone happens to keep and store the snapshot, nothing bad should happen either, it may mean that load()'s degrade internally to load_full()'s which is equivalent to just using a load_full() in the first place.
| #[derive(Debug)] | ||
| pub struct TrackingFileSystem<'a>(pub &'a mut BTreeSet<PathBuf>); | ||
|
|
||
| impl<'a> serde_vars::source::FileSystem for TrackingFileSystem<'a> { | ||
| fn read(&mut self, path: &std::path::Path) -> std::io::Result<Vec<u8>> { | ||
| self.0.insert(path.to_owned()); | ||
| std::fs::read(path) | ||
| } | ||
|
|
||
| fn read_to_string(&mut self, path: &std::path::Path) -> std::io::Result<String> { | ||
| self.0.insert(path.to_owned()); | ||
| std::fs::read_to_string(path) | ||
| } | ||
| } |
Basically the moment all overrides are applied right now. Later once we have hot reloads, the config stays mutable, it may be reloaded at any point.
Rule of thumb: Store
|
3 participants
Most of the changes here are because of the introduction of the
ConfigSnapshotwhich is obtained from theConfig.A lot of the code now requires a snapshot instead of the config. Most actual changes are contained to
relay-config/src/config.rs.This also includes a change to actually start tracking all the dependencies of the config, which is still unused yet, but is a preparation for making the config hot reloadable. I started with this before I realized how many other changes this will require.
Also happened to see some leftovers in the config, which I cleaned up (e.g. cardinality limiting config).
There are still a few rough corners I want to tackle:
ServiceStatewhich I think should be something likeCurrentServiceStatewith a snapshot of the config.ProjectCacheServicegrabs a bunch of values on init and never refreshes them. This could be all made reload aware.UpstreamProjectSourceServicealso caches a few values that could be made reload-able.UploadServicealso caches some values on startup.HttpServerprobably should own a snapshot to make it explicit that nothing can be reloaded here.relay-configand also be validated on reload.And of course the actual reloading still needs to be implemented.
There should be absolutely no behavior change.