3722 embeds vizjson in redis #3733
Conversation
|
Frontend tests were OK 👍 (details) |
|
Frontend tests were OK 👍 (details) |
| format.html { render layout: 'application_public_visualization_layout' } | ||
| format.js { render 'embed_map', content_type: 'application/javascript' } | ||
| if @cached | ||
| response.headers = @cached[:headers] |
There was a problem hiding this comment.
I think you can't overwrite response.headers, since it might already have a value (a quick local check says (byebug) response.headers {"Access-Control-Allow-Origin"=>"*", "Access-Control-Allow-Methods"=>"*"}). Maybe a merge! will be fine.
There was a problem hiding this comment.
Good catch, actually I see in the response it added two Cache-Control headers.
There was a problem hiding this comment.
The duplicate entry comes from holding both :"symbolized" and "plain string" keys
|
👍 apart from the comments. |
|
I would add a test (mocking the cache, but to see that works fine), but apart from that +1 |
|
Frontend tests were OK 👍 (details) |
|
Frontend tests were OK 👍 (details) |
| @redis = redis_cache | ||
| end | ||
|
|
||
| def get(visualization_id) |
There was a problem hiding this comment.
Maybe I'm over-thinking, but after a second thought, if Redis went down or there were a error or connection issue this would fail and it wouldn't fallback to normal rendering, maybe this class should have error handling: reporting it but not propagating the exception.
There was a problem hiding this comment.
- varnish down => ok, serve through redis cache
- varnish and redis down => we're f**ked
- redis down => sets and invalidations would fail
There was a problem hiding this comment.
Also needed for vizjson cache, maybe I will make a separate branch for that that will conflict a little with the branch refactoring it out of old and new models :S
There was a problem hiding this comment.
if redis go down, don't worry, everything will go down
|
Frontend tests were OK 👍 (details) |
|
Frontend tests were OK 👍 (details) |
3722 embeds vizjson in redis
@javisantana @Kartones @juanignaciosl can you please review?
I wish I knew a bit more about rails internals and its "convention over configuration", I really had to struggle with headers and
renderin order to send the cached response.