feat: Adds secondary sqlite db to ads-client - #7567
Conversation
| pub placement_id: PlacementId, | ||
| pub ad_type: StorableAdType, | ||
| pub ad_body: Vec<u8>, | ||
| } |
There was a problem hiding this comment.
This struct isn't strictly needed at this moment, but it makes the test generation a bit cleaner, and I could see it being useful for the next PR in the process (which will likely utilize conversions from AdImage, etc, to a more generic structure- this one)
| pub struct AdsStoreBuilder { | ||
| db_path: PathBuf, | ||
| max_size: Option<ByteSize>, | ||
| } |
There was a problem hiding this comment.
We keep max_size in the builder but it's not publically exposed ATM, as discussed. Options like this will be accessible by surfaces as needed.
| pub store_config: Option<AdsStoreConfig>, | ||
| pub context_id_provider: Option<Box<dyn super::ContextIdProvider>>, | ||
| pub environment: Environment, | ||
| pub telemetry: T, |
There was a problem hiding this comment.
nit: we could keep alphabetical order here, I have written an article about alphabetical sorting if you are curious why I think it's important https://almaju.github.io/blog/docs/fundamentals/style/sorting
There was a problem hiding this comment.
(also there are other places of the diffs where we could keep alphabetical sorting but I only commented here)
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This code feels like a lot for just storing the ad type. I think there are other ways we could explore.
- We could store it as an enum like:
#[derive(Serialize, Deserialize)]
#[serde(tag = "type")]
enum Ad {
Image(Image),
Tile(Tile),
}and then we can use the type field in the json?
-
Or we could even use something like
std::any::type_name::<MyStruct>()to get the type as a string? -
I feel like we would know the type at deserialization anyway so instead of those wrapper type we could just have something like:
fn lookup<T: Deserialize>(...) -> Result<T>There was a problem hiding this comment.
Oh! And I realize that code lives under mars which should only be JSON API logic, not store logic!
There was a problem hiding this comment.
I like that we put these common things out but I feel database is not the right name, bytesize and clock are unrelated to database, I would rename it to something like "core", "primitives", "common", "support" etc.?
There was a problem hiding this comment.
Or we could even move them to ads_store directly if we expect they will be only used by ads_store in the future?
2 participants
This adds an 'adstore' equivalent to the to be used for storing ads in the new stateful ads-client design. It does not provide any FFI or interfaces or hooks or anything, simply the sqlite queries and internal logic, mimickingmost of the logic for the
http_cache. This uses a separate sqlite db to the http_cache one, though they are functionally quite similar. The tests are all similar/the same, except for removed ones (such as TTL tests, see below).In particular, this does not include:
http_cache(we will have a background worker thread updating + clearing) it's not included in this PR .http_cachehasrecord_http_cache_outcomefor cache hits/misses)I did, however, keep the following modules:
max_sizewill still be relevant here)Pull Request checklist
[ci full]to the PR title.