getjob() doesn't seem to work

[bttmly]

My guess is probably because addjob() needs a "FROM" argument. There's probably a relatively simple workaround with the ioredis Command, I'll submit a PR once I have that working.

I might work on adding some basic tests to run against a real local disque server – if I wrote those up would you merge them? It would provide at least a way to know the basic commands work.

On another note, even though Disque is still "beta" it'd be great to have a solid Node client for it. Given all the excellent work on ioredis, basing the Disque client on that foundation seems excellent. I'm wondering though if you'd be open to refactoring this package a bit – ideally the Node Disque client could inherit directly from the Commander class, or perhaps another, new, intermediate class that does the work around setting up the offlineQueue and commandQueue. Just a thought – let me know, I'd be happy to contribute.

Thanks!

[luin]

When I started this project, I ran this client against Disque and it worked. After that, I decided to updated this client again once Disque become stable. However, your point makes sense that even Disque is still in the beta stage, a working client is still important.

It would be great if you have time to refactor this project and I just moved this project to a independent org.

I'm also wondering whether we can inherit directly from the Commander class. However, there are several problems. First, ioredis doesn't expose the Commander class since Commander is designed to be used internally. Second, Commander contains a little Redis-specified logic (e.g. https://github.com/luin/ioredis/blob/master/lib/commander.js#L23-L24). I think maybe we can write the Disque client from scratch, probably based on some ioredis code, or separate some small modules that can be used by both ioredis and this disque client.

[bttmly]

Looking through ioredis a little more it seems like there's an awful lot of instantiation and connection logic that would need to be replicated in a disque client.

What about another approach -- just inheriting from the Redis class directly. Am I wrong in thinking that the main reason you didn't do that was to support connecting to one of a list of possible servers? If so perhaps we could override the connect in the Disque subclass. I'll dig through later on to see how feasible that is, but if you have thoughts on this please let me know.

[luin]

Just had a look at the source code of this repo (I had forgotten how the client was implemented 😆 ), and it turned out we've already had Disque inherited Redis class. I think this might be a good approach to avoid replicating the connection logic since Disque is similar (e.g. using a same protocol, sharing many networking codes) to Redis.

[bttmly]

Right right, the client does inherit from Redis, but it seems like that instance never actually connects. It maintains another internal instance, and uses that one to issue commands, etc. I was looking to avoid that indirection, if there were another way to handle connecting to one of a list of servers. I'm not too familiar with the Connector class that handles connecting, but I can take a look.

[bttmly]

I had something in mind like this https://github.com/nickb1080/diskew/blob/master/lib/index.js but obviously this naive version can only connect to one server. Haven't found a good way to fallback to another server while maintaining only one client instance -- seems that a single Disque/Redis instance will be closely tied to it's internal connector instance. If the logic in ioredis that sets this.connector was exposed somehow (taking parameters) it would be doable, but that seems kind of ugly.

[bttmly]

Reviewing the ioredis source, I had a thought. If the logic of creating this.connector (https://github.com/luin/ioredis/blob/c5502704ddaa14bfaf5490cbd84eef9997f44818/lib%2Fredis.js#L112-L116) were instead moved inside Redis.prototype.connect, it would help greatly for Disque subclassing it and potentially connecting to a different server if the first one isn't reachable.

[luin]

Moving that snippet inside Redis.prototype.connect is definitely possible. And will it make implementing our Disque client easier if we allow user passing their own Connector subclass as a constructor option?

I've not looked deeply into Disque since I'm a little busy recently. I'll find some time next week for this library 🚀 .

[bttmly]

Yeah no rush, I'm using the simplified implementation above for some basic testing, don't need advanced connection options now. Not sure we even need to subclass Connector, but if connect() took options (defaulting to this.port and this.host) we'd be able to do something along the lines of

Disque.prototype.connect = function () {
  var self = this;
  var endpoints = self.endpoints.slice();
  return new Promise(function (resolve, reject) {

    function _connect () {
      var endpoint = endpoints.shift();
      if (endpoint == null) {
        return reject(new Error("All endpoints were unreachable"));
      }

      Redis.prototype.connect.call(self, endpoint.host, endpoint.port)
        .then(resolve)
        .catch(_connect);
    }

    _connect();

  }).then(function () {
    // post-connection logic, like setting self.stream or whatever
  })
}