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

Added config option for specifying cacheKey namespace #84

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Spamercz
Copy link
Member

@Spamercz Spamercz commented Oct 1, 2018

You can configure cache namespace.

Examples:

  • New version of application uses new namespace.
  • Application development versions can have different namespaces in same database.

@enumag
Copy link
Member

enumag commented Oct 3, 2018

Can you check #56? It seems similar and there was some discussion about such feature there.

@enumag
Copy link
Member

enumag commented Oct 3, 2018

Also you created #83 with the same name - if it's a duplicate too, please close one of them.

@Spamercz
Copy link
Member Author

Spamercz commented Oct 3, 2018

We have two uses for this

  • our CI server is building test version of application for every PR. There is little trafic but hundreds of instances and convinient use of neon configuration to separate them would be great. Also we use different redis databases for different projects not different versions of application.
  • in deploy proccess, we disable server for pulic, build new version and warm up cache. There would be perfect if we can warm up to new namespace, because we have redis server shared with multiple app servers. But I can see it helps even if deploy is less complicated and you just want to with new request clean cache.

Overal this solution is less complicated than #56 and optional, you dont have to use it.

We tried Redis partitioning but it proved much too complicated to configure and use on daily basis.

Also I did not implement RedisLua version because Lua is not my friend and in high trafic applications we are experiencing issues with journal locking, but we were unable to pinpoint where problem is.

@enumag
Copy link
Member

enumag commented Oct 3, 2018

Just to be clear I just wanted to point you to the related issue but I won't work on this. My role in Kdyby is mainly merging bugfixes or small features. I won't merge this since it's not a bug-fix and also because Filip was reluctant to include similar feature in #56. You will have to ask him.

@fprochazka
Copy link
Member

fprochazka commented Oct 4, 2018

our CI server is building test version of application for every PR. There is little trafic but hundreds of instances and convenient use of neon configuration to separate them would be great. Also we use different redis databases for different projects not different versions of application.

I would consider this a valid use-case, but you can just as easily just run 100s of instances of redis and it would take almost no additional RAM. Even without docker, just start it on different ports. This would also make your testing environment better, since it would increase the isolation level of separate application instances.

in deploy proccess, we disable server for public, build new version and warm up cache. There would be perfect if we can warm up to new namespace, because we have redis server shared with multiple app servers. But I can see it helps even if deploy is less complicated and you just want to with new request clean cache.

This use-case has been addressed in #56 (comment)

Overal this solution is less complicated than #56 and optional, you dont have to use it.

It's a bit less complicated - yes. The implementation in #56 is also optional.


I still think my arguments against namespacing the keys are very relevant.

This is a highly power-user feature. Since you can really easily shoot yourself in the foot with this.

I'm not sure what to do.

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

Successfully merging this pull request may close these issues.

3 participants