Config Singleton #4220
Config Singleton #4220
Conversation
|
Thanks a lot @janusz-anue ! We'll take a look in the next days. FYI, ever since your last commit one more exe has been added in |
There was a problem hiding this comment.
Thanks, looks good @janusz-anue. One point for discussion in the singleton design. WDYT?
|
You'll also need to format the code, see https://github.com/valhalla/valhalla/blob/master/CONTRIBUTING.md#code-contributions for pre-commit which makes it a bit easier IMO. |
…halla into config-singleton
valhalla/configuration.h
Outdated
| static config_singleton_t instance(config_file_or_inline); | ||
| return instance.config_; |
There was a problem hiding this comment.
just move this down into the namespace level function below and remove this function. we dont need two ways to do the same thing, do we?
There was a problem hiding this comment.
(you'd need to add the namespace function as a friend function of this class to make that work i guess)
There was a problem hiding this comment.
Added the function as a friend, needed to declare the function before defining it, that's why I added the declaration above the singleton
valhalla/configuration.h
Outdated
| @@ -0,0 +1,49 @@ | |||
| #ifndef VALHALLA_CONFIGURATION_H_ | |||
There was a problem hiding this comment.
can we move this whole headers content to config.h? i konw its currently got the version information in it but i think this is perfectly fine to be in there too
There was a problem hiding this comment.
Yeah, after doing that I needed to resolve some naming collisions (config function and config variable in anonymous namespace) in some testifies. I resolved them by just renaming the var to cfg. Hope that's ok
|
When playing around with the new singleton I was noticing that when used with a long inline config the check to see if the argument is a file or inline config threw an exception. This is now mitigated in the new try-catch block in the constructor. Additionally I was finding that when I read the singleton in the GraphTile for example, many tests will fail because the tests (that use the GraphTile in any way) logically don't initialise the singleton. So I thought to create a new exception for when the singleton is not initialised that can be caught and handled accordingly. I can expand on that in another (draft) PR for a configurable live speed fading. Another solution could be to add a member var that is true when the config is initialised, otherwise false. |
|
Hi @kevinkreiser and/or @nilsnolde, could you please take another look at this PR so that we can use the config singleton to complete the original task #4177 . For the speed fading the PR is prepared but i would wait to open it until the merge of this PR. |
CHANGELOG.md
Outdated
| @@ -23,6 +23,7 @@ | |||
| * ADDED: support for `:forward` and `:backward` for `motor_vehicle`, `vehicle`, `foot` and `bicycle` tag prefixes [#4204](https://github.com/valhalla/valhalla/pull/4204) | |||
| * ADDED: add `valhalla_build_landmarks` to parse POIs from osm pbfs and store them as landmarks in the landmark sqlite database [#4201](https://github.com/valhalla/valhalla/pull/4201) | |||
| * ADDED: add primary key in the landmark sqlite database and a method to retrieve landmarks via their primary keys [#4224](https://github.com/valhalla/valhalla/pull/4224) | |||
| * CHANGED: the boost property tree config is now read in to a singleton that doesn't need to be passed down anymore [#4220](https://github.com/valhalla/valhalla/pull/4220) | |||
There was a problem hiding this comment.
would you mind moving this to the bottom of the list
There was a problem hiding this comment.
ill just move this and merge so as not to hold this up any longer. very sorry for the wait
| @@ -30,7 +30,7 @@ using rp = rapidjson::Pointer; | |||
|
|
|||
| namespace { | |||
|
|
|||
| const auto config = test::make_config("test/data/utrecht_tiles"); | |||
| const auto cfg = test::make_config("test/data/utrecht_tiles"); | |||
There was a problem hiding this comment.
yeah this is an inconvenience. not being able to name something config when inside the valhalla namespace, the good news is that in a future pr we wont need to even keep the variable around we can just always call config() whereever we are in the code (in most scenarios)
| config_singleton_t() = delete; | ||
| config_singleton_t(const std::string& config_file_or_inline) { | ||
| if (config_file_or_inline.empty()) { | ||
| throw valhalla::ConfigUninitializedException(); |
There was a problem hiding this comment.
why not just use throw std::runtime_error("Config singleton was not initialized before usage");? it must be because we have try catch blocks in things like the workers which key off the type of exception? perhaps the workers have a new catch clause that returns a specific error when not configured? or maybe it should be a valhalla_exception_t with a specific error code? fwiw this is fine, dont feel the need to change it, i more just wanted to discuss it/think about it a little.
Fixes #1158
Sorry for the long wait. I have finally gotten around tackling this PR. As discussed in #4177 and in #1158, this PR solves the first phase of implementing the configuration as a singleton and changes the top-level passings. This means that basically the arg_parse is changed and every class using it, as well as the valhalla_service.
Later in another phase the other passings can be removed too.
Before using config() the singleton should be configured with a config file or an inline config string.
I am open for suggestions and also nitpicks as I am looking to improve my c++ coding.
Janusz Spatz on behalf of Mercedes-Benz Tech Innovation GmbH.
Provider Information