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

Feature key prefixes #56

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

vallbo
Copy link

@vallbo vallbo commented Nov 23, 2015

Adding possibility to create a keys with prefix, because of multiple instances using one Redis server.

@@ -1,2 +1,3 @@
vendor
composer.lock
.idea
Copy link
Member

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleted in 27657e1

@fprochazka
Copy link
Member

This is what databases are for, no?

@vallbo
Copy link
Author

vallbo commented Nov 25, 2015

Yes, it is an option for using redis with multiple instances of one application.
The question about multiple database was also discussed on the StackOverflow (http://stackoverflow.com/questions/16221563/whats-the-point-of-multiple-redis-databases).
At the moment application is using wrapper for this library and working without any problems.
But i think it will be more elegant if option of key prefixes will be directly in this library.

@hrach
Copy link

hrach commented Nov 25, 2015

Databases have limited count + its possible to distinguished them only by a number (impossible to use it for simultanous app revisions, etc.)

@vallbo
Copy link
Author

vallbo commented Dec 14, 2015

@fprochazka Is there some progress, because our company is waiting for this pull request. Thank you for answer.

@hranicka
Copy link

👍 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);
Copy link
Member

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 :)

@fprochazka
Copy link
Member

Also, I'm gonna have to ask you to write tests for this.

@fprochazka
Copy link
Member

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_"
Copy link
Member

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

@fprochazka
Copy link
Member

Now that I've reviewed your PR. Let me repeat few important facts

  • I still think increasing the max databases count and having separate database for session and separate database for cache for each application is better then namespacing keys. You cannot call flushdb on deploy when you wanna clear cache, if all your applications are sharing a database. You have to call keys instance1_ and pipe that to del command. Which basically murders redis performance. Using only one database doesn't scale! It's best to have an instance of redis for session and another instance for cache-related data.
  • Redis is single threaded => if your application manages to block the only instance that the other 20 applications are using, you've managed to kill 20 applications. Remember that once you've started session, it blocks the request untill it acquires the key, which it cannot do if the instance is blocked. It can only use one cpu. This can effetively cause multiple waiting requests in PHP - they will queue up untill all the workers are bussy and your webserver will start returning errors. This escalates wery easily.
  • Redis has basically nonexistent memory footprint and running 2 instances per application on one machine consumes very little memory. Read Redis Partitioning they're mostly writing about paritioning one dataset of one application but it's interesting.
  • If your cache and sessions combined from 20+ applications in one database is not a performance problem, or inconvinience for you, you probably don't need redis and filesystem cache is more then enough for you.

@hrach
Copy link

hrach commented Jan 9, 2016

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 :-)

@juniwalk
Copy link

juniwalk commented Jan 9, 2016

👎 for me, using single database for multiple apps sounds really bad practise to me.

@fprochazka
Copy link
Member

@hrach

Either way, I'd strongly suggest having a separate instance for cache (and another one for sessions) for each application.

@enumag
Copy link
Member

enumag commented Jan 10, 2016

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. 👎

Vladimir Bosiak added 3 commits February 15, 2016 17:30
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
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.

6 participants