seomoz / qless

Queue / Pipeline Management
MIT License
293 stars 78 forks source link

Cannot get Qless up and running - too difficult to install #168

Open ronalchn opened 10 years ago

ronalchn commented 10 years ago

I've only just recently put Sidekiq into my Rails application, and when I looked at Qless, I saw that many of the features would be useful. My context is to use it for CPU-intensive jobs, which take 2 seconds - several minutes each. This means that the queuing part of it doesn't have to be so fast, and Qless, with the extra book keeping, provides useful statistics, so the O(log n) data structures it uses seems ideal.

Looking through the readme, it was obvious that there are many rough edges, but I decided to try to integrate it. The rough edges include the fact that gems normally present a simple, canonical way to install and configure the gem. That is, to say, place a line in your Gemfile, create an config/initializers/qless.rb file, etc. However, there is a lot of setup code (eg. client = Qless::Client.new) where it is not specified where it is normally placed.

It is also frustrating that there seems to be different places to configure the redis connection, and different ways it must be done. For example, the Qless::Client.new(redis options here), and the Rakefile environment variables.

After following the steps, I wasn't able to run rake qless:work, getting this error:

uninitialized constant Qless::Worker

And this is after a lot of time was spent searching the codebase for where this mystery class is specified. No amount of grepping would reveal where the code is, and there doesn't appear to be any code connecting the rake task to the "*Worker" classes. The environment variables asked for in the Rakefile also do not appear to be used anywhere in the gem.

It appears that perhaps the master branch is a non-working version, or something strange is happening. In fact, the readme does not say which branch or version of the gem is the latest working version.

It almost seems that this gem has had a lot of work stuffing it with features, but no work on making it easy to use, or obvious how to get it working in a canonical way. Each section of the readme is too focused on the feature it is describing, but the readme does not have a clear structured narrative of how to quickly get started, and integrated into an existing application - such as a Rails application.

myronmarston commented 10 years ago

@ronalchn -- thanks for taking the time to try Qless out and caring enough to report your problems rather than just moving on.

Are you using the Qless gem from rubygems.org or using Qless HEAD from github? Things are a bit different between the two. The fact that you're getting uninitialized constant Qless::Worker suggests you may be using Qless HEAD from github, as that constant no longer exists but the rake task in HEAD still references it. Actually, I believe @dlecocq (based on conversations I've had with him) wants to remove the rake task and move to a stand-alone binary instead -- there's no strong reason it needs to be a rake task and a binary can be a lot more flexible. On our projects we start the worker through a bit of code such as:

qless = Qless::Client.new(redis: redis_connection)
queues = %w[ foo bar ].map { |name| qless.queues[name] }
job_reserver = Qless::JobReservers::ShuffledRoundRobin.new(queues)
worker = Qless::Workers::ForkingWorker.new(queues).run

(Note that this only works against github HEAD; for the released gem I believe the rake task should still work).

As for instructions for integrating into Rails: the reason we don't have that is because we're not using Qless with any rails projects. Our projects that use Qless are using sinatra for the HTTP bits. The general "here's how you use Qless with a ruby project" instructions (which I agree could be fleshed out some more) should work with Rails; for Rails there'll probably be some standard places you put the bits of code (such as a config initializer). Anyhow, we're happy to accept a PR to improve our docs with respect to Rails.

Let us know if you have any other pain points; we definitely want to solve them.

ronalchn commented 10 years ago

Ok, I decided to have another go at it.

I had to change the code you gave me:

namespace :qless do
  task :work => :environment do
    require 'qless/job_reservers/ordered'
    require 'qless/worker'
    queues = %w[judge].map { |name| $qless.queues[name] }
    job_reserver = Qless::JobReservers::Ordered.new(queues)
    worker = Qless::Workers::ForkingWorker.new(job_reserver, :num_workers => 2, :interval => 2).run
  end
end

Also, put in pull request #169 so that the password is used.

Other than that - can the redis version dependency be increased? It is 2.2, which means that Redis.connect is used (deprecated in the latest versions), but 3.0 was released a long time ago, and anyways, your Travis-CI build matrix doesn't test 2.2.

Also, instead of making my own global variable - $qless = Qless::Client.new, can we add those instance methods as class methods which keeps it's own instance of the client? For example:

class Qless::Client
  def self.establish_connection(options = {})
    @client = self.new(options)
  end
  def self.client
    establish_connection if @client.nil?
    @client
  end
  def self.queues
    client.queues
  end
  ...
end

I like the idea of using Qless::Client.method rather than $qless.method.

You might also want to think about adding connection pool support, just because it is quite hard for anyone to use a connection pool properly if they are using a multi-threaded server, but it can be as simple as changing your Client#call method to be wrapped in a connection pool. What makes it hard is that every other object is given a direct reference to client (which all call the Client#call method). And it means anyone wanting to do this would need to trawl through the internals to work out what is happening.

myronmarston commented 10 years ago

Other than that - can the redis version dependency be increased? It is 2.2, which means that Redis.connect is used (deprecated in the latest versions), but 3.0 was released a long time ago, and anyways, your Travis-CI build matrix doesn't test 2.2.

It's >= 2.2, which declares that Qless works with version 2.2+ of the redis gem. It works fine with 2.2 and 3.0. If we changed the gemspec to say 3.0 (or ~> 3.0, or whatever), it would not allow someone to install and use Qless with redis 2.2 (as bundler wouldn't be able to resolve the Gemfile). I don't see a reason to stop people from being able to use Qless with the older redis version.

You mention our travis build matrix doesn't test 2.2, and you're right. It would be good to have it continue to run builds against 2.2 but we haven't put the effort into that.

Also, instead of making my own global variable - $qless = Qless::Client.new, can we add those instance methods as class methods which keeps it's own instance of the client? For example:

The lack of a global Qless::Client.client attribute is an intentional design decision. It's actually one of the key features that I think differentiates Qless from Sidekiq and Resque: both of the latter have the global client instance that you've asked for, but what that means is that you cannot easily scale them beyond a single redis instance, since every process has a single global sidekiq or resque client instance. At Moz we have several projects that have such a high volume of jobs and workers (as an example: one project is running about 3K workers in prod), that we've needed to scale beyond a single redis instance. N redis instances can store N times the number of jobs that one can and process N times the throughput. This is trivial to do with Qless precisely because it lacks the global attribute you asked for. We simply shard our jobs based on the campaign id (analogous to the user id in our system). We even have a sharded UI gem that provides a single Web UI to look at all the sharded Qless instances.

You might also want to think about adding connection pool support, just because it is quite hard for anyone to use a connection pool properly if they are using a multi-threaded server, but it can be as simple as changing your Client#call method to be wrapped in a connection pool. What makes it hard is that every other object is given a direct reference to client (which all call the Client#call method). And it means anyone wanting to do this would need to trawl through the internals to work out what is happening.

It's unclear to me what advantage this would provide us. Writing multi-threaded code to work with Qless is trivial: just use a separate Qless::Client instance per thread (each of which could have its own redis connection). Qless::Cilent instances are quite lightweight.

evanbattaglia commented 10 years ago

Is the rake qless:work task going to be fixed or removed at some point? Seems weird for the documentation to mention a non-working task since November...