Add Redis support

[marcelaraujo]

I added Redis support and I will write the necessary tests yet.

[Baachi]

Maybe you should support the predis client, too.

[marcelaraujo]

So, I must be implement a factory to allow Redis support two different extensions?

[Baachi]

The Redis and the Predis client has nearly the same API. You should remove the typehint from the constructor and in the constructor check if $redis is a \Redis or \Predis\Client instance, if not you should throw a exception.

[rosstuck]

This uses string keys with json encoding to store the data; would there be any reason to not use the Redis' hash type instead? Then we could support partial updates/fetches and allow other clients to operate on the data more safely.

[Baachi]

@rosstuck I would prefer use redis native hash type, because is faster than json_encode.

[benbender]

ping @beberlei Is there any reason why this is not merged yet? Would love to test redis support without composer-hacks :+1:

[beberlei]

@benbender I am waiting on feedback regarding the redis hash type vs json discussion. I am not very deep into redis, so i am not very capable of answering this, but @rosstuck and @Baachi mentioned hash support which works better here?

[benbender]

@beberlei OK, I see the point. If we use hmset/hmget here, we could fetch/update partial objects. (But I dont see a use-case for this yet.) As bonus there would be a slight performance benefit by saving the json_encode/json_decode calls. And it would be more "redis-style" to use hmset/hmget.

ping @marcelaraujo Can you please have a look on that matter so this can be merged?

[rosstuck]

I've opened a quick PR against @marcelaraujo's fork which starts the process but I haven't tested it yet. If he doesn't have time to pick it up, I'll give it some more testing tomorrow (at WhiskyWeb today) https://github.com/marcelaraujo/KeyValueStore/pull/1

[marcelaraujo]

Hey guys

I need help to do the tests files. My experience is very large on Redis development but some tests where is needed work with mockups, I have no enough acknowledge to make it.

And about the changes on Redis process like the the use of json_encode or native Redis encoding, the Redis extension doesnt allow you encode an object directly.

Try it by yourselves.

Thanks everyone!

[texdc]

ping @marcelaraujo @rosstuck @beberlei : status update?

[rosstuck]

For what it's worth, I did some follow-up research earlier this year. @marcelaraujo is right, this won't work when deserializing objects. To support that, we'd have to get a bit fancier. A few possibilities:

1) We track the serialized objects inside a separate key of the hash. However, since we can't do push/pop operations on a subkey, we'd have to fetch the key, deserialize it, append to it, and overwrite that key entirely. That's pretty doable, although there's a slight chance of it running out of sync when doing a partial update.

2) We put the serialized keys into a set (entirely separate key). Should solve the previous problem, but we'd need to issue two commands (no real penalty if we put them in a Redis transaction) but it feels overly complicated.

3) We simply don't support objects on this driver or we require they be annotated properly. I prefer the latter but it's something that would only be caught at runtime, I reckon. Personally, I wonder if allowing partial metadata mapping is worth the hassle but that's outside the scope of this ticket.

Perhaps it would be best to allow @marcelaraujo's original PR to stand and revert my pull to switch the type over. I think hashes would be better here, especially when using Redis with another non-PHP client, but the original PR is a good implementation and something working is better than nothing. A hash based driver could always be added as a separate object in the future if the above solutions sound too complicated. I apologize for delaying the merge.

[marcelaraujo]

Sorry guys!

I'm still working with Redis but I've been busy for a long time and I don't care if anyone takes this little piece of KeyValueStore project forward without me.

What do you think about it?

[marcelaraujo]

Done, I did a reset hard on my last commit and the merge was removed.

[beberlei]

@marcelaraujo Sorry for the delay cough, merged :)

[marcelaraujo]

Today I will review my codes and write the missing tests.

I'm glad that my PR has been accept. Thank you guys and let me know if you need anything