Repository navigation
AC-220: Complete request implementation for stateful client - #7758
thesuzerain wants to merge 4 commits into
Conversation
thesuzerain
changed the title
| error_support::error!("Running a request is currently not set up yet: {self:?}"); | ||
|
|
||
| match self { | ||
| // TODO: All of the MARS options are defaulted, as the AdsClient options refactor is required first. |
There was a problem hiding this comment.
This PR will conflict with this PR.
Whichever one gets merged second will have the MARSRequestOptions properly passed through here.
| #[cfg(feature = "stateful")] | ||
| #[derive(Clone, Debug, Default)] | ||
| pub struct WorkerConfig { | ||
| pub worker_batch: Option<Duration>, |
There was a problem hiding this comment.
This refers to worker batch duration (eg: batch for 10 seconds). I'll have more clear documentation on the Moz-public struct version of this, matching the existing pattern for these.
There was a problem hiding this comment.
I think it's too early for that. As a V1 I don't think we'd do batching yet?
There was a problem hiding this comment.
I agree, but this isn't publically exposed or used yet.
Most of the underlying structure already exists + fleshed out on this (eg: see discussions about RequestQueue here or here), from before we decided to be more agile about this (develop end to end first). So this config provides the field to the BackgroundWorker::new() that already exists.
If you prefer I can create a version (eg: enum variant, or if a boolean or None is passed in) whereRequestQueue exists but does not batch? That way we can ensure no batching happens in V1 (I think I'd do that in a separate PR though).
| DispatchRequest::RequestAds { ad_requests } => { | ||
| ads_client_inner.prefetch_ads_request( | ||
| ad_requests.into_iter().collect(), | ||
| MARSRequestOptions::default(), |
There was a problem hiding this comment.
worker should not know about MARS specific things
what is MARS related should stay with MARS
if we need request options here then this means that's a domain struct so no need to prefix it with MARS
| pub fn builder<P: AsRef<Path>>(db_path: P) -> AdsStoreBuilder { | ||
| AdsStoreBuilder::new(db_path.as_ref()) | ||
| AdsStoreBuilder::new(Some(db_path.as_ref())) | ||
| } | ||
|
|
||
| pub fn builder_memory() -> AdsStoreBuilder { | ||
| AdsStoreBuilder::new(None::<String>) | ||
| } | ||
|
|
There was a problem hiding this comment.
I feel that this defeat the whole point of builders, you now have two constructors for two different cases. The builder is meant for that?
AdStore::builder().database(path).build()
There was a problem hiding this comment.
The reasoning for my choice here to do builder_memory was that we always expect a path to be provided- it generally isn't a configurable option (I remember pushback against having path=None be available as an option, and I think also pushback against something like in_memory() be a builder function available to create a memory database). We obviously need to be able to create a in-memory database 'on purpose' if there's no config passed.
I liked this setup because, as a user, have to provide a path with builder or explicitly say builder_memory() (it's a fully different kind of database, and you would need to think about it).
I'm totally fine with doing it the way you've suggested, if we are fine that AdStore::builder().build() makes an in-memory database. I can make that change along with the one below.
|
|
||
| pub fn store_ads(&self, ads: HashMap<PlacementId, Vec<Ad>>) -> Result<(), FetchAdsError> { | ||
| for (placement_id, ad) in ads { | ||
| self.holder.store_ad(&placement_id, &ad)?; |
There was a problem hiding this comment.
we cannot do multi insert in sqlite? 🤔
There was a problem hiding this comment.
Our module currently doesn't have that ability- it uses the same logic the HTTPCache implementation and has a single insertion method. It definitely can though, I'll do that separately.
| AdsStore::builder_memory() | ||
| .build() | ||
| .inspect_err(telemetry::record_ads_store_error) | ||
| .ok() |
There was a problem hiding this comment.
didn't we already have that logic in the AdsStore builder?
Also this is not a concern for the client: the client needs an AdsStore, it doesnn't care how it was made, that's a technical concern for the AdsStore itself.
There was a problem hiding this comment.
This is different behavior. This is for if "no store config exists at all" (eg: no entry in AdsClientConfig), not if it fails. We don't want to force store config to be passed as that's a breaking change.
Similar to what is being done here with .max_size(max_cache_size) and .default_ttl(default_cache_ttl), but falling back to a specific db if we don't have a config. It looks like the other fallback functions because we don't have any builder functions like max_size on the AdsStore yet.
Also this is not a concern for the client: the client needs an AdsStore, it doesnn't care how it was made, that's a technical concern for the AdsStore itself.
I'm not sure I agree. That's true for any kind of fallback mechanism- that should be kept in AdsClient but this is how to handle no proper entry in the AdsClientConfig. "If there's no config, do this instead. "
We could refactor the HttpCacheBuilder to not be a builder, and instead take a singular AdsCacheConfig field, if we are concerned about the logic being here, but I do quite like the builder setup.
If you prefer, I can change the pattern for both to look something like this (example given with HttpCache, not tidied up):
let default_cache_ttl = client_config.store_config.
// This requires `builder` to be able to accept `None` for a memory database.
let path = cache_cfg.map(|x| db_path).clone();
// No cache config is passed, but we make it anyway with default values.
let cache_cfg = client_config.cache_config.unwrap_or_default();
let default_cache_ttl = Duration::from_secs(
cache_cfg
.default_cache_ttl_seconds
.unwrap_or(DEFAULT_TTL_SECONDS),
);
let max_cache_size =
ByteSize::mib(cache_cfg.max_size_mib.unwrap_or(DEFAULT_MAX_CACHE_SIZE_MIB));
let mut http_cache = HttpCache::builder(cache_cfg.db_path);
// Pass database path if exists, otherwise, if no config was created, we fall back to a memory database.
if let Some(path) = path {
http_cache = http_cache.database(path);
}
http_cache = http_cache.max_size(max_cache_size)
.default_ttl(default_cache_ttl)
.build()
.inspect_err(telemetry::record_build_cache_error)
.ok();
(Or similarly, but having builder be able to accept None as a short-hand for a memory path, or unwrap_or to :memory: here)
Allows background worker to successfully run + insert into the AdsStore.
This was tested with a 'full' FFI implementation, but I extracted these components to reduce PR load.
Not included/TODO:
No new tests, but integration tests in FFI layer have been run and will be included in FFI layer PR.
Pull Request checklist
[ci full]to the PR title.