Add support for configuring Restate in benchmarks via env variables #597
Conversation
src/benchmarks/src/lib.rs
Outdated
| let default_config = ConfigurationBuilder::default() | ||
| .worker(worker_options) | ||
| .meta(meta_options) | ||
| .build() | ||
| .expect("building the configuration should work"); | ||
|
|
||
| config | ||
| let temp_dir = tempfile::tempdir().expect("temp directory needs to be available"); | ||
| let configuration_path = temp_dir.path().join("restate.yml"); | ||
| let configuration_file = std::fs::File::create(configuration_path.as_path()) | ||
| .expect("creating the configuration file should work"); | ||
|
|
||
| serde_yaml::to_writer(configuration_file, &default_config) | ||
| .expect("serializing the config should not fail"); |
There was a problem hiding this comment.
Perhaps avoid this dance and simply add a method in Configuration like load_only_from_env :)
There was a problem hiding this comment.
And use os::set_env for rocskdb and meta storage paths.
There was a problem hiding this comment.
True, that would be a tad bit easier. What I don't like so much about then env variable only approach is that it will silently break if we change the names of config options whereas now, the benchmark would stop compiling.
There was a problem hiding this comment.
What I would love to have is a merge function between Configurations. That would probably be the easiest solution but it is not supported (needs manual implementation).
There was a problem hiding this comment.
You could also do that, just copy the current Configuration::load function in Configuration::merge_with_env. The logic would be the very same of load except you remove the loading from file.
There was a problem hiding this comment.
That's a good idea. I think I'll extend load to take a default configuration. That way we keep the exact same logic for how to load the configuration for benchmarks as well as for the binary.
|
Thanks for the review. Merging this PR now. |
This fixes #596.