all() why KEYS and MULTI?

[naholyr]

I have an existing implementation of all() method, but before submitting a PR I want to discuss the matters, one I'm sure of, the other one I may have missed something.

Using KEYS is bad

As the Redis documentation clearly states, KEYS should be avoided in production code: it blocks the entire database during computation. I alread had issues with this, it's not to be underrated, a thousand keys can be enough to block almot a second, which means your database will quickly stack delays and the whole system will fall down. This is serious business ;)

The solution is to use consecutive calls to SCAN (maybe in a MULTI?).

Why using MULTI + GET instead of MGET?

I've seen you were calling MULTI then one GET per session key, and finally execute the transaction. But why not using directly MGET? Is there a specific reason I'm not aware of?

Code sample

My current implementation (the one I used before #all() landed here):

RedisStore.prototype.all = function (cb) {
  var self = this;
  var sessions = {};
  (function nextBatch (cursorId) {
    self.client.scan(cursorId, 'match', self.prefix + '*', 'count', 100, function (err, result) {
      if (err) {
        return cb(err);
      }

      var nextCursorId = result[0];
      var keys = result[1];

      // Need dedupe, as scan can return duplicates
      var fetchKeys = keys.filter(function (key) {
        return !(key in sessions);
      });

      if (!fetchKeys.length) {
        // No more keys, end now
        return cb(null, _.values(sessions));
      }

      self.client.mget(fetchKeys, function (err, jsons) {
        if (err) {
          return cb(err);
        }

        jsons.forEach(function (json, i) {
          var id = fetchKeys[i];
          try {
            var sess = self.serializer.parse(json.toString());
            sess.id = id;
            sessions[id] = sess;
          } catch (err) {
            // skip failed sessions
          }
        });

        if (nextCursorId != 0) {
          // next batch
          nextBatch(nextCursorId);
        } else {
          // end of cursor
          cb(null, _.values(sessions));
        }
      });
    });
  })(0);
};

[wavded]

/cc @anotherpit

[anotherpit]

@naholyr
The only specific reason KEYS and MULTI + GET were used is my poor knowledge of Redis :—)

[anotherpit]

@naholyr
I suppose implementation of ids() method could also be improved based on your ideas, right?

[twelve17]

Hello--
Thanks for the work in putting this npm together.

I am wondering if it is on the roadmap to get @naholyr 's feedback into the codebase? I am not super versed yet with Redis but would be willing to help with testing if necessary.

[wavded]

I'm open to a PR @twelve17 / @naholyr

[naholyr]

OK on it, I'll check if ids() would enjoy the change too and get back to you

[wavded]

Thanks @naholyr !

[wavded]

Resolved in 3.3.0