Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[spike] Shop product list pagination impact on cache solution #4039

Closed
2 tasks done
daniellemoorhead opened this issue Jul 15, 2019 · 9 comments
Closed
2 tasks done
Assignees

Comments

@daniellemoorhead
Copy link
Contributor

daniellemoorhead commented Jul 15, 2019

Question to be answered

Can the cache solution we use to load products continue to be used in a world where the product list is paginated and the search/filter functions continue to work?

Work to be done in 4 hour timebox

  • determine if current cache solution can handle proposed pagination feature ('Y/N')
  • determine any changes required for cache if the answer to above is 'N'
@sauloperez
Copy link
Contributor

sauloperez commented Jul 15, 2019

determine if current cache solution can handle proposed pagination feature

TL;DR: not it can't.

The main problem of the cache implementation is that keys do not take pages into account. They currently follow the pattern products-json-:distributor_id-:order_cycle_id, meaning that a request like /shop/products?page=2 would return whatever was cached for an earlier request such as /shop/products?page=1 (or any other previous one). Therefore, cache keys need to take the page parameter into consideration.

What becomes complex then is cache expiration. Adding a new product into an order cycle would require expiring all cached pages and as far as I know, Memcached has no wildcards, so expiring them all with something like products-json-:distributor_id-:order_cycle_id-page-* won't be possible. So it could be the time to adopt a different cache expiration policy.

I've given a quick look at key-based cache expiration and it would solve the problem while removing the need to expire keys manually thus improving our codebase a GREAT deal while fixing lots of subtle bugs. I'll take a deeper look and post my findings.

@luisramos0
Copy link
Contributor

interesting!

what about using a cache entry for the number of pages?
products-json-:distributor_id-:order_cycle_id-number_of_pages

I tried to rewrite RefreshProductsCacheJob.perform (not a valid programming language 🤣 ):

Screenshot 2019-07-15 at 22 43 49

@daniellemoorhead
Copy link
Contributor Author

Where to next with this @sauloperez? There is additional time required to look at key-based cache expiration, and also the suggestion from @luisramos0. Do we need to do an additional timebox to take this to a conclusion?

@sauloperez
Copy link
Contributor

sauloperez commented Jul 16, 2019

@daniellemoorhead I want to take a quick look at key-base cache expiration given that I spent just 2:30 on it so far :trollface: .

To answer your suggestion @luisramos0 I'm afraid that's not possible in our use case. I purposefully avoided some details but as @daniellemoorhead described in #4041 and #4040, the shop product list we'll have to accept filters and search terms, therefore the number of pages is something dynamic that can't be known in advance. It depends on the request parameters: filter, search terms even per_page (if we let clients specify it).

That leads us to a more (traditional) caching design where the first request the server receives while the cache is empty will be served from DB and then be cached. Subsequent request would cause a cache hit and so would be much quicker. Given the reduced number of items on a page, that 1st response would still be substantially quicker than loading all products.

We need to include all these parameters in the cache-key for this to work but again we could remove all cache-expiration logic, which is a massive upside (there are lots of lines of code dealing with this).

Does it make sense?

@Matt-Yorkley
Copy link
Contributor

Did we define that "API Exemplar"?

#actionsfromtheglobalgathering

@luisramos0
Copy link
Contributor

with search terms... this is not a straight forward issue...
could we build this solution without cache first and then add cache if required? because it's paginated, the queries will be much smaller and faster.

@Matt-Yorkley api examplar is an issue that didnt get to dev ready yet, it's just next, after banc contact spike.

@Matt-Yorkley
Copy link
Contributor

I think paginated but not cached would be a big leap forward 👍

@luisramos0
Copy link
Contributor

luisramos0 commented Jul 19, 2019

in my large screen, the shop fits max 5 products without scrolling, so we could make pages of 5-10 products for the shopfront. and I bet 90% of the page views will just load the first page (no scroll).

@sauloperez
Copy link
Contributor

of course, if we can manage without caching it would be ace. Only numbers will tell so I put this on hold with the hypothesis that won't be needed. Once a first PoC without pagination is implemented will check the numbers and come back to it if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants