added initial version of search APIs #25
Conversation
There was a problem hiding this comment.
Really good job, @rathijitpapon ! 🙌
A couple of comments here and there, but there are only 3 things I want to see before we merge:
- user id foreign key in the new table and mock the user id.
- send real http request to our fastapi backend (while we do not have gRPC solved).
- A couple of tests
server/src/search/routes.rs
Outdated
| services::insert_search_history(&pool, &mut connection, &search_query, &search_response) | ||
| .await?; |
There was a problem hiding this comment.
I don't know the performance characteristics of the two insertions (cache and pg). They are both async, so it shouldn't be a problem, but if we see something in the future this could be a good candidate for the spawn_blocking_with_tracing util in telemetry.rs.
| cache | ||
| .set( | ||
| &search_query.query, | ||
| serde_json::to_string(&search_response) | ||
| .map_err(|_| eyre!("unable to convert string to json"))?, | ||
| ) | ||
| .await | ||
| .map_err(|e| AppError::from(e))?; |
There was a problem hiding this comment.
Noice!
We'll find an approach to do this as simple as cache.set(key, value).await? later 👌
| cache | ||
| .zremrangebyrank( | ||
| "search_history", | ||
| 0, | ||
| -SETTINGS.cache_max_sorted_size as isize - 1, | ||
| ) | ||
| .await | ||
| .map_err(|e| AppError::from(e))?; |
There was a problem hiding this comment.
It looks like this removes history with rank between (0, negative max sorted size], correct?
What is the purpose of doing that? 🤔
There was a problem hiding this comment.
Yeah. It's used to remove all the unnecessary search queries from the sorted set. This way we can use smaller storage for this.
No description provided.