-
Notifications
You must be signed in to change notification settings - Fork 5
feat: initialize suggestion providers at start up #193
Conversation
Moves initialization of suggestion providers to startup and shares the initialized data between threads. Closes #127
…247-remote_settings
TL:DR; moved the provider initialization to the async startup, outside of the app. Two tests are failing because the app isn't reading the remote settings URL because the data is already cached(?). |
I think this is spot on. We'll need a way to resync the data from the server. Maybe we could add a |
Yeah, mostly working out how the TestingTools are actually failing, but that might be a rabbit hole. |
|
||
#![allow(unused_imports)] |
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 import is unused, and if so why do we need it?
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.
There are a number, including:
crate::utils::test_tools::TestReqwestClient,
crate::TestingTools
anyhow::Result
merino_settings::providers::*
request::StatusCode
std::collections::*
As noted in the comment above, setting #![cfg(test)]
causes the compile to fail because merino-integration-tests::utils::test_tools::merino_test
cannot find crate::suggest
, presumably because crate::suggest
is not compiled as part of the non test build. So, instead, I enabled suggest, with the assumption that the remaining, non-test function setup_remote_settings_collection(...)
is fine being part of the release build.
merino-settings/src/lib.rs
Outdated
// note `#[cfg(test)]` will cause a compile error as the | ||
// `merino_test` proc_macro will not be able to find this | ||
// field. | ||
pub test_changes: Option<Vec<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'd think this should be nested under the Remote Settings configuration object, instead of at the top level, or at least mention that it is for Remote Settings. Also some documentation about what it is and how to do it would be warranted.
merino-suggest/src/domain.rs
Outdated
@@ -39,6 +39,7 @@ impl Proportion { | |||
} | |||
} | |||
|
|||
/// Define helper impls for float values |
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.
heads up: I think this might conflict with some other PR, but I forget which one.
…247-remote_settings
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.
missed that other clone on the SuggestionProviderRef
that could be removed
…247-remote_settings
Moves initialization of suggestion providers to startup and shares the
initialized data between threads.
Closes #127