Open Genaker opened 4 years ago
The non-use of the SESSION_PREFIX
constant was actually intentional because I experienced odd cases where during shutdown the class constants had already been unloaded by PHP and then it reported an error that it was not defined or something strange like that..
I strongly recommend against sharing one Redis instance between Magento instances. The problem is that with shared resources one instance could affect all of the other instances negatively, for example by exhausting available memory and causing evictions for other clients. Also, you will actually get better performance using one Redis instance per Magneto installation since Redis is single-threaded and the Magento instances are excellent and natural "sharding" points.
With Docker it is so incredibly simple to setup additional redis servers. E.g. if using docker-compose:
services:
redis-session:
image: redis:5-alpine
command: redis-server --appendonly yes
volumes:
sessions:/data
volumes:
sessions:
Or if not yet using Docker you can still easily do it:
$ docker run -p 127.0.0.1:63790:6379 --name redis-session_1 -d -v redis-session_1:/data redis:5-alpine redis-server --appendonly yes
$ docker run -p 127.0.0.1:63791:6379 --name redis-session_2 -d -v redis-session_2:/data redis:5-alpine redis-server --appendonly yes
$ docker run -p 127.0.0.1:63792:6379 --name redis-session_3 -d -v redis-session_3:/data redis:5-alpine redis-server --appendonly yes
$ docker run -p 127.0.0.1:63793:6379 --name redis-session_4 -d -v redis-session_4:/data redis:5-alpine redis-server --appendonly yes
The overhead for running separate instances is so miniscule it is basically negligible (~2MB).
Another advantage is you can set different memory limits for each instance and also easily move one dataset to a different server without affecting the others.
Lastly, the longer your prefix the more memory your indexes will consume.
You are right, but this parameter will be configurable if somebody doesn't want use he will not use it. We can't run docker Redis because we using AWS ELASTICCACHE and we need just this parameter logically separate sessions keys, we don't care about the performance of the session because it is jus single request per page and we need it for development environment - to have shared session for 1-20 development environments managed by aws elastic beanstalk. This code doesn't affect the existing user's performance. I can move this Const (SESSION_PREFIX and PREFIS_ADDITIONAL) to the class property (this->prefix) to avoid constant issues.
I see, thanks for the additional info.
A couple other thoughts:
If you change the constant back I'll consider merging it, but it still seems unnecessary to me.
The problem is not random cookie, Session key it is not human-readable. DB instance ID is not human-readable. After environment delation, we also want to remove all garbage. We need to remove the cookies associated with that environment (most of the environments are temporary). With DB ID we can't do that because we can't generate a proper id for each environment we need to build an additional system to keep an association environment with DB ID add keep documentation and teach new developers how to associate env with a new ID. In the case in the prefix, we will have a human-readable session key like sesionstaging$sessionID. Also, AWS Redis by default has 16 DB limits.
Session constant removed.
Use another ElastiCache instance? This promotes a bad practice IMO.
Where you can see bad practices there? One instance per different keys is ok. Different instances produce price and management overhead. Especially if you are building something big like SaaS or Cloud one Redis is better 10.
In big cloud environments, one Redis/keyDB instance used for many Magento installations. This prefix is necessary to properly manage sessions. Thank you for merging these changes