-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat(settings): Add periodically updating runtime config options #251
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #251 +/- ##
==========================================
- Coverage 83.35% 83.30% -0.05%
==========================================
Files 19 20 +1
Lines 3568 3624 +56
==========================================
+ Hits 2974 3019 +45
- Misses 594 605 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f576c6f
to
be0e779
Compare
src/main.rs
Outdated
@@ -50,6 +51,9 @@ async fn log_task_completion(name: &str, task: JoinHandle<Result<(), Error>>) { | |||
async fn main() -> Result<(), Error> { | |||
let args = Args::parse(); | |||
let config = Arc::new(Config::from_args(&args)?); | |||
let runtime_config = Arc::new(RuntimeConfigManager::new( | |||
config.runtime_config_path.clone(), | |||
)); |
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.
Will this get pushed into the InflightActivationStore
?
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.
If we eventually require the it to need runtime configs, then yes. Otherwise, dropping tasks via killswitching happens outside of InflightActivationStore
src/runtime_config.rs
Outdated
pub struct RuntimeConfigManager { | ||
pub config: RwLock<RuntimeConfig>, | ||
pub path: String, | ||
} |
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.
I think it might be worthwhile to just let the RuntimeConfigManager
also take ownership of the green thread that polls the file. Something like
pub struct RuntimeConfigManager {
...
handle: JoinHandle<()>
}
impl RuntimeConfigManager {
pub fn new(path: String) -> Self {
...
handle: tokio::spawn(async {
// file IO here
});
}
impl Drop for RuntimeConfigManager {
fn drop(&mut self) {
self.handle.abort();
}
}
So we don't need to schedule it on main anymore
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.
What is the advantage of scheduling the tokio thread inside RuntimeConfigManager
rather than in main alongside the maintenance task? If we put this in the config manager, how do we call it periodically?
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.
For me the main advantage is that RuntimeConfigManager
is a self contained thing that doesn't rely on another thing to make it work properly. Right now, the contents of RuntimeConfigManager
is only correct if something else is always calling reload_config
on it.
What is the advantage of scheduling the tokio thread inside
RuntimeConfigManager
rather than in main alongside the maintenance task? If we put this in the config manager, how do we call it periodically?
You don't need to call it periodically anymore, since it reloads on its own in the green thread it owns.
impl RuntimeConfigManager {
pub fn new(path: String) -> Self {
handle: tokio::spawn(async {
loop {
sleep(...).await;
// file IO here
}
});
}
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.
I see, I'm okay treating it as an actual "manager" which spawns itself own tokio thread and is in-charge of polling itself.
You don't need to call it periodically anymore, since it reloads on its own in the green thread it owns.
Oops, "calling it periodically" were the wrong words. I meant having the IO work execute periodically. In your snippet, we'd sleep right? Is there a difference between sleeping and using interval.tick()
? Do both yield execution to the scheduler?
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.
In your snippet, we'd sleep right? Is there a difference between sleeping and using interval.tick()? Do both yield execution to the scheduler?
Yup, both will yield execution to the scheduler because they're both using .await
. You're right it's probably better to use interval
instead of sleep
here because interval
will take into account how long the file IO took.
The difference between interval
and sleep
is that sleep
is yielding execution for the duration of its parameter, where as by default interval fires using the system clock. For example
loop {
a();
sleep(Duration::from_secs(1).await;
}
vs
let timer = time::interval(Duration::from_secs(1);
loop {
a();
timer.tick().await;
}
In the former, it does not matter how long calling a()
will take. Once a()
finishes, we will sleep for 1 second. But in the second example, timer.tick().await
will take until it has been 1 second since last time timer.tick().await
has returned. This gets kind of complicated when a()
takes longer than 1 second and if we configure the MissedTickBehavior.
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.
Also circling back to the original conversation, I think if we want to drive the update of RuntimeConfig
explicitly in main, it's also fine since we're doing that with upkeep
anyway. So it maybe better to follow existing patterns.
src/config.rs
Outdated
@@ -116,6 +119,7 @@ impl Default for Config { | |||
kafka_auto_offset_reset: "latest".to_owned(), | |||
kafka_send_timeout_ms: 500, | |||
db_path: "./taskbroker-inflight.sqlite".to_owned(), | |||
runtime_config_path: "./runtime_config_defaults.yaml".to_owned(), |
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.
Do we want this to be an Option instead, so are we expecting that this file will always exists? If so, let's add it to the integration tests because the rebalancing one is failing from not being able to find this file
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.
Right, now that we have defaults, we don't need to read from a default file anymore. Updated!
This PR is responsible for adding runtime configs to taskbroker. The path to the yaml file is defined under
Config.runtime_config_path
. This can be used to override with a ConfigMap in k8s. Otherwise, defaults are defined underruntime_config_defaults.yaml
. The runtime config manager reloads the configuration file into memory as part of the maintenance task (which currently runs every 60 seconds).