3344 version vizjson redis keys #3726
Conversation
- Extract caching logic to a class - Version keys
|
Frontend tests were OK 👍 (details) |
| def purge_redis_vizjson_cache | ||
| vizs = CartoDB::Visualization::Collection.new.fetch(user_id: self.id) | ||
| redis_http_keys = vizs.map{ |v| v.redis_vizjson_key(https_flag=false) } | ||
| redis_https_keys = vizs.map{ |v| v.redis_vizjson_key(https_flag=true) } | ||
| redis_vizjson_cache = CartoDB::Visualization::RedisVizjsonCache.new($tables_metadata) |
There was a problem hiding this comment.
Maybe you could move move most of this code to a simpler del(vizs) method inside RedisVizjsonCache.
There was a problem hiding this comment.
Sorry but that is done for performance reasons when purging all the keys, which is the only purpose. Instead of sending 1 request per viz this sends just one per user, as requested bu the CTO 😄
There was a problem hiding this comment.
Yep, I supposed that (we don't want one redis connection per viz), but I think those lines fit better inside the redis cache class, since this looks like caching logic.
|
👍 |
…n-redis-keys Conflicts: app/controllers/admin/visualizations_controller.rb app/models/visualization/member.rb
|
Frontend tests were OK 👍 (details) |
|
Frontend tests were OK 👍 (details) |
|
Frontend tests were OK 👍 (details) |
|
Frontend tests were OK 👍 (details) |
|
Frontend tests were OK 👍 (details) |
|
Frontend tests were OK 👍 (details) |
|
Frontend tests were OK 👍 (details) |
|
@juanignaciosl & @Kartones test should pass now. Changes since the last time you reviewed it are basically updating with master, move the cache code to app/helpers and correct a couple of minor things. If everything goes fine my plan is to release this tomorrow morning. |
| def purge_redis_vizjson_cache | ||
| vizs = CartoDB::Visualization::Collection.new.fetch(user_id: self.id) | ||
| redis_http_keys = vizs.map{ |v| v.redis_vizjson_key(https_flag=false) } | ||
| redis_https_keys = vizs.map{ |v| v.redis_vizjson_key(https_flag=true) } | ||
| redis_vizjson_cache = CartoDB::Visualization::RedisVizjsonCache.new() |
There was a problem hiding this comment.
I don't like forcing User to know the internals of vizjson cache. For example, he must know about http-https versioning. I would extract the next 4 lines to a method inside RedisVizjsonCache and replace them with the call, something like redis_vizjson_cache.purge_visualizations_cache(vizs). Nevertheless I think we've already discussed about this, so if there's a god reason for keeping it just ignore this 😅
|
👍 (comment is just a matter of personal preferences, nothing relevant). |
|
Frontend tests were OK 👍 (details) |
|
Frontend tests were OK 👍 (details) |
3344 version vizjson redis keys
@juanignaciosl @Kartones please review
Not the best code ever but avoids duplication. If you see any major impediment I will postpone it till Monday and focus on 1st level cache of embeds, otherwise I'd like to have this in master asap to continue on top of it.
Thanks!