Introduce Redis::OPT_PACK_IGNORE_NUMBERS option.
#2616
Conversation
|
cc @remicollet This does seem to work but let me know if anyone sees edge cases that might cause problems. A few come to mind.
So perhaps we disallow the flag for certain serializers and/or compression. |
Why is this a problem? Shouldn't we then also skip on unserialize/uncompress? Aah everything is a string that redis returns, that's the problem? Selectively allowing this flag is not nice in my honest opinion. It would mean that libraries would need to add special logic to either use phpredis internal logic or do additional work when another serializer/compressor is set. I guess there is also no way to pass a flag on per key basis to let phpredis know whether this needs to be serialize/compressed or unserialized/uncompressed. If there is no safe way to add this feature independent of config, maybe then lets not do it at all? |
|
@michael-grunder what are you gonig to do when users will start complain that they set integer/double and receive string back? 😄 $redis->setOption(Redis::OPT_PACK_IGNORE_NUMBERS, true);
$redis->setOption(Redis::OPT_SERIALIZER, Redis::SERIALIZER_PHP);
$redis->set("pi", 3.1415);
var_dump($redis->get("pi")); // string(6) "3.1415" |
Well thats the behaviour they can expect when they explicitly set this flag, users need to be aware that this flag has some extra costs and implications. It basically then behaves the same as without any settings for numbers. You also get a string back. Because they explicitly set this option, they will have to cast the result properly. It is not a breaking change. What worries me is that we can not get consistent results from all serializers/compressors. If that is not possible, we can then leave this feature out :) |
Adds an option that instructs PhpRedis to not serialize or compress numeric values. Specifically where `Z_TYPE_P(z) == IS_LONG` or `Z_TYPE_P(z) == IS_DOUBLE`. This flag lets the user enable serialization and/or compression while still using the various increment/decrement command (`INCR`, `INCRBY`, `DECR`, `DECRBY`, `INCRBYFLOAT`, `HINCRBY`, and `HINCRBYFLOAT`). Because PhpRedis can't be certain that this option was enabled when writing keys, there is a small runtime cost on the read-side that tests whether or not the value its reading is a pure integer or floating point value. See #23
b619684 to
759cf73
Compare
|
@yatsukhnenko I added a bit of read-side logic. I think it's starting to approach a somewhat sane feature but let me know what you think. |
* We want to run the logic if either a serializer OR a compression option is set. * IEE754 doubles can theoretically have a huge number of characters.
|
@TheLevti: Are you gonna open a PR to Laravel? |
I will add the configuration option once its released, so that users can at least have control about this behavior and decide for themselves how to best make use of it. Regarding those specific usecases in the framework, currently I think its not necessary to force set it as my previous fix ensured we disable serialization/compression when used with specific features (e.g. rate limiting). I believe its better to state in the docs that those settings should be used for caching purposes only and not for any other higher level behavior. Of course over time we could slowly add support for it in specific components (like queues, where it could benefit from big payload compressions). But last time I checked, we rely too heavily on LUA scripting in the queue component, so I would advise people to not use such a connection for that. |
|
@TheLevti Can you ping me in the PR? I'll add it to the Relay driver as well. |
Adds an option that instructs PhpRedis to not serialize or compress numeric values. Specifically where
Z_TYPE_P(z) == IS_LONGorZ_TYPE_P(z) == IS_DOUBLE.This flag lets the user enable serialization and/or compression while still using the various increment/decrement command (
INCR,INCRBY,DECR,DECRBY,INCRBYFLOAT,HINCRBY, andHINCRBYFLOAT).Note that PhpRedis cannot know when reading data back, so ther will still be a cost incurred as we attempt and then fail to deserialize and/or uncompress the value.
This is particularly problematic for
COMPRESSION_LZ4as we use a small 5 byte header as a sanity check which could happen to pass depending on the specific numeric value stored.See #23