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

Support for HSET/HGET feature #53

Open
ghost opened this issue Oct 2, 2016 · 11 comments
Open

Support for HSET/HGET feature #53

ghost opened this issue Oct 2, 2016 · 11 comments

Comments

@ghost
Copy link

ghost commented Oct 2, 2016

To my knowledge, the HSET/HGET will have better performance when there are many keys share the prefix, just like this:

cache:user:session:1
cache:user:session:2

My suggestion is to store the everything after the last / symbol as the HSET/HGET key. For example, we should turn:

GET cache:user:session/1

to:

HGET cache:user:session 1

automatically.

If you like this idea, I will push you a PR.

@ghost
Copy link
Author

ghost commented Oct 2, 2016

And also, this is a compatible solution for other cache-manager store implementations.

@mrister
Copy link
Contributor

mrister commented Oct 3, 2016

@kcauchy I think It is good Idea, maybe you could do a PR for us to check out the implementation, what do you think @PuKoren ?

@PuKoren
Copy link
Contributor

PuKoren commented Oct 3, 2016

Agree, looks good!

@ghost
Copy link
Author

ghost commented Oct 19, 2016

It seems not able to be done.

There are two problems:

  1. ttl is not supported for redis HGET/HSET. Which means I have to disable it for keys as hash table entries. You can't set ttl for entire hash table, that will "expire" every entries.
  2. keys are not supported.
    • Redis provides HKEYS, but no pattern supported.
    • If you try to get cache:session:*, there could be entries like cache:session:db/1 and cache:session:user:1, then you must get wrong values for keys commands, because all other keys under cache:session:db will be ignored. So the only correct solution is to run KEYS first, then run every entry HKEYS (because we don't know whether it is a hash table or not.) That is totally unbearable for performance.

Seems that is a design problem for node-cache-manager.

@jeff-kilbride
Copy link
Contributor

Automatically converting the existing SET/GET commands to HSET/HGET is not a good idea. HSET/HGET are associated with the hash data type in redis and most commonly represent objects with field-value pairs:

> hmset user:1000 username antirez birthyear 1977 verified 1
OK
> hget user:1000 username
"antirez"
> hget user:1000 birthyear
"1977"
> hgetall user:1000
1) "username"
2) "antirez"
3) "birthyear"
4) "1977"
5) "verified"
6) "1"

This is not a good option for a simple key/value store like cache-manager for the reasons noted above -- no ttl / expires command for individual values and no easy way to scan the keys.

To support the hash data type, as well as the other data types redis offers, it would be better to have something like a raw method that accepted the name of a redis command and it's parameters (possibly as an array), and just passed them along to the underlying connection.

redisCache.raw(
  'hmset', 
  ['user:1000', 'username', 'antirez', 'birthyear', '1977', 'verified', '1'],
  function(result) {
    // result is 'OK'
    redisCache.raw('hgetall', 'user:1000', function(result) {
      // result is the object returned by node_redis
      // { username: 'antirez', birthyear: '1977', verified: '1' }
    });
  });
);

In this way, we could support virtually all of the underlying redis commands. However, the user would be responsible for ensuring the command and parameters were correct, and for knowing the return value.

@mrister
Copy link
Contributor

mrister commented Aug 21, 2017

Very nice observation by @jeff-kilbride, however even us when we use this module we more frequently store objects then we do simple string values, so It might be worth supporting out of the box, or make it configurable, I very much like the idea with the raw approach for flexibility of usages.
Some more reading: https://stackoverflow.com/questions/13557075/redis-set-vs-hash

@mrister
Copy link
Contributor

mrister commented Aug 21, 2017

@kcauchy still interested in doing the PR ?

@ghost
Copy link
Author

ghost commented Aug 21, 2017 via email

@mrister
Copy link
Contributor

mrister commented Aug 21, 2017

@kcauchy. Ok thanks, might you show us this system if it is open source? :-)

@ghost
Copy link
Author

ghost commented Aug 21, 2017

Sure...

@ghost ghost closed this as completed Aug 21, 2017
@mrister
Copy link
Contributor

mrister commented Aug 21, 2017

I'll reopen this in the meantime for further discussion, @dial-once/developers thoughts?

@mrister mrister reopened this Aug 21, 2017
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

No branches or pull requests

3 participants