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

[WIP] New powerfull feature zlink #92

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

[WIP] New powerfull feature zlink #92

wants to merge 2 commits into from

Conversation

sam2x
Copy link
Collaborator

@sam2x sam2x commented Feb 24, 2015

Hello,

Since now 15days, i'm using Nohm as a Model Abstraction to redis.
After some practise and several tests cases, i decided to keep it
as a main Redis interaction for my project.

Here the benefits i can highlight :

  • NodeJS-oriented, easy to deploy
  • Maintain code more easily than the raw Redis Node API.
  • Model definition is complete & powerfull:
    • Type & Validation are elegants manner to ensure user-input data.
    • A set of essential options are available for properties (index, unique; defaultvalue, etc.)

I also fall in some unpleasant issue that should be imho redesigned :

  • Not using Redis Sorted Set feature at all on main Nohm feature (except Sort)
  • Ambiguous context of documentation, i needed to create several code test
    • Model context during loading (this)
    • Difference about nohm.factory, nohm.models, etc, and suited-use explication

As Sorted Set is one of the best benefit of Redis, i decided to integrate it in the relation model feature.
My pull request allow to specify a score in the zlink function :
User.zlink(Action, { score : date })

Remaining things to do :

  • Since i use the proxied function Link, a warning should be raised when score is used with 'link' and not 'zlink'. (else the score will be inserted as item in the classical SET).
  • zunlink / zunlinkByScore
  • relGetRangeByScore (relName, options { min(0), max, withscores_, limit_, offset_, count_}
  • relGetRevRangeByScore
  • others relevant redis function of sorted set (http://redis.io/commands#sorted_set)

What is the benefit ?

  • Powerfull Redis Type for fast retrieve
  • Native Redis API SSET function based on the score (ZRANGEBYSCORE)
    • Limit feature
    • Score fast search

Now in ONE call, we give relation powerfull search :
get object id of Y linked to object X, which have [min/max] score S [limit the result to L] [DESC/ASC]

What do you think ?

Well, this is a small contribution. I think there is other improvement to bring about some Redis feature(eg: sscan/zscan).
Anyway thank you for your project, it is truely helping. Please don't let it go ;)
I hope to have time to help you again in the near future.

Regards,
S

@sam2x
Copy link
Collaborator Author

sam2x commented Feb 24, 2015

The dbVals code added is misplaced, he should placed during multi (since the number of parameter differs between zadd and sadd) :

        async.forEach(dbVals, 
          function (val, next) {
            var multi = client.multi();
            if (type === 'zadd' || type === 'zrem')
            {
              val.score = options.score
              multi[type](Nohm.prefix.relationKeys+val.keyStore, val.score, val.key);
              multi[type](val.key, val.score, val.id);
            }
            else {
              multi[type](Nohm.prefix.relationKeys+val.keyStore, val.key);
              multi[type](val.key, val.id);
            }
            multi.exec(next);

Also, as you likely noticed, my code related to the fireevent is incorrect.
About GetByScore :

    /**
   * Retrieves all sorted set relations to another model  .
   */
  exports.getByScore = function getByScore(objName, options, name) {
    var callback = h.getCallback(arguments),
    self = this;
    name = name && typeof name !== 'function' ? name : 'default';
    if (!this.id) {
      Nohm.logError('Calling getAll() even though this '+this.modelName+' has no id. Please load or save it first.');
    }
    args = []
    args.push(this.relationKey(objName, name))
    if(options.rev)
    {
      // ZREVRANGEBYSCORE key max min [WITHSCORES] [LIMIT offset count] 
      options.max !== undefined ? args.push(options.max) : args.push('+inf')
      options.min !== undefined ? args.push(options.min) : args.push('-inf')
      options.withScore ? args.push("WITHSCORES") : 0
      options.offset !== undefined && options.count !== undefined ? args.push("LIMIT", options.offset, options.count) : 0

      this.getClient().zrevrangebyscore(args, function(err, value){
        callback.call(self, err, value);
      })
    }
    else{
      // ZRANGEBYSCORE key min max [WITHSCORES] [LIMIT offset count] 
      options.min !== undefined ? args.push(options.min) : args.push('-inf')
      options.max !== undefined ? args.push(options.max) : args.push('+inf')
      options.withScore ? args.push("WITHSCORES") : 0
      options.offset !== undefined && options.count !== undefined ? args.push("LIMIT", options.offset, options.count) : 0

      this.getClient().zrangebyscore(args, function(err, value){
        callback.call(self, err, value);
      })   
    }
  }

@sam2x sam2x changed the title New powerfull feature zlink (not finished) [WIP] New powerfull feature zlink May 25, 2017
@sam2x
Copy link
Collaborator Author

sam2x commented May 25, 2017

Patch incomplete, i added a scan/zscan feature with zlink that i didnt push (important for search stuff, etc.). I need to clean up my code and push my change.

@maritz
Copy link
Owner

maritz commented Mar 26, 2018

Same problem as #104 I think this should be converted into a new feature for v2.x (probably 2.2 or 2.3), instead of attempting to put it into v0.9 and then porting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants