-
Notifications
You must be signed in to change notification settings - Fork 70
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
Feature key prefixes #56
base: master
Are you sure you want to change the base?
Conversation
@@ -1,2 +1,3 @@ | |||
vendor | |||
composer.lock | |||
.idea |
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.
Add this to your gitignore_global, not here.
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.
deleted in 27657e1
This is what databases are for, no? |
Yes, it is an option for using redis with multiple instances of one application. |
Databases have limited count + its possible to distinguished them only by a number (impossible to use it for simultanous app revisions, etc.) |
@fprochazka Is there some progress, because our company is waiting for this pull request. Thank you for answer. |
👍 for option of key prefixes. In the current master there is no way of custom keys specification. Only hard-coded "Nette" prefixes are present via constants (eg. https://github.com/Kdyby/Redis/blob/master/src/Kdyby/Redis/RedisStorage.php#L29). |
$cacheStorage = $builder->addDefinition($this->prefix('cacheStorage')) | ||
->setClass('Kdyby\Redis\RedisStorage'); | ||
->setClass('Kdyby\Redis\RedisStorage', $constructParams); |
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.
you should upgrade your PhpStorm, the new one doesn't break the indentation in here :)
Also, I'm gonna have to ask you to write tests for this. |
Also, before you continue. Wouldn't it make more sense to implement this on the client level? Just asking, I know it would be much more complex. |
Instance 2 | ||
```yml | ||
redis: | ||
keyPrefix: "instance2_" |
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.
Now that I think about it, someone might misinterpret this as adding the prefix to the client object - it would be a little more writing, but it would also make a lot more sense to me having them under the specific sections.
parameters:
redis:
namespace: instance1
redis:
host: 127.0.0.1
port: 6379
journal:
namespace: %redis.namespace%
session:
namespace: %redis.namespace%
storage:
namespace: %redis.namespace%
debugger: off
Now that I've reviewed your PR. Let me repeat few important facts
|
How do you solve continuos deployment when you have multiple instances. You can't flush the old db, you have to have another separated storage. Not advocating this solution, just asking :-) |
👎 for me, using single database for multiple apps sounds really bad practise to me. |
Either way, I'd strongly suggest having a separate instance for cache (and another one for sessions) for each application. |
I don't use Redis yet because of this very problem - databases are numbered and making sure that each app uses a different database is a real pain. That said I have to agree with @fprochazka. This doesn't seem to be the correct solution to me. 👎 |
using namespace for different types of storage add mock extension for testing (mockery/mockery)
[ERROR] Kdyby/Redis/DI/RedisExtension.php:54 Mixed tabs and spaces indentation
[ERROR] KdybyTests/Redis/RedisNamespaceStorage.phpt:29 Mixed tabs and spaces indentation [ERROR] KdybyTests/Redis/RedisNamespaceJournal.phpt:28 Mixed tabs and spaces indentation
Adding possibility to create a keys with prefix, because of multiple instances using one Redis server.