-
Notifications
You must be signed in to change notification settings - Fork 652
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
Cache vizjson in redis to avoid hitting DB #2324 #2194
Conversation
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) |
@javisantana review please. A few notes:
|
end | ||
|
||
describe '#redis_cached' do | ||
it "Uses the block given to calculate a hash if there's a cache miss" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kartones as a curiosity, this is how I solved the called block problem
Frontend tests were OK 👍 (details) |
@rochoa can you please review? |
retest this, please |
@@ -20,7 +20,8 @@ | |||
tables_metadata: 0, | |||
api_credentials: 3, | |||
users_metadata: 5, | |||
redis_migrator_logs: 6 | |||
redis_migrator_logs: 6, | |||
visualizations: 7, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we using another database for this? 😈
I understand how this can be useful, but unfortunately I consider
Redis multiple database errors my worst decision in Redis design at
all... without any kind of real gain, it makes the internals a lot
more complex. The reality is that databases don't scale well for a
number of reason, like active expire of keys and VM. If the DB
selection can be performed with a string I can see this feature being
used as a scalable O(1) dictionary layer, that instead it is not.
https://groups.google.com/forum/#!msg/redis-db/vS5wX8X4Cjg/8ounBXitG4sJ
Frontend tests were OK 👍 (details) |
OK for me 👍 although I would try to avoid using an extra database for this. |
Ok, I'll move it to |
I agree with you on the naming issue but that's easier to fix than having to migrate another database in the future (yes, I know probably we won't migrate to just one database anytime soon but better to avoid making this the standard). |
As discussed in peer review, using a new redis db should not be the standard for new code. See https://groups.google.com/forum/#!msg/redis-db/vS5wX8X4Cjg/8ounBXitG4sJ
Frontend tests were OK 👍 (details) |
Cache vizjson in redis to avoid hitting DB #2324
Ready to be merged