queues_idx is not persistent, could lead to starvation #13

Closed
mperham opened this Issue Feb 10, 2012 · 8 comments

2 participants

@mperham
Owner
mperham commented Feb 10, 2012

In manager.rb#L103 queues_idx is reset to 0 on every dispatch, which means that queue will be unfairly weighted. queue_idx should be @queue_idx and act like a ring buffer index so we start from where we left off in the last dispatch.

@ryanlecompte
Contributor

I'm wondering if the logic in Manager#dispatch could actually be simplified a bit. In CLI, we add all of the queues to options[:queues], duplicating the items based on the weights. So if someone configures the server with -q foo,5 and -q bar,2 we would have ['foo', 'foo', 'foo', 'foo', 'foo', 'bar', 'bar']. After that we shuffle the array via Array#shuffle!. I'm wondering if you can still get the weighted randomization effect in Manager#dispatch by simply grabbing a random element from @queues instead of iterating/indexing and (perhaps redundantly checking?) each queue in @queues for work. Essentially Manager#dispatch would do something like this:

  def dispatch(schedule)
    loop do
      found = false
      @ready.size.times do
        found ||= find_work(@queues.sample)
      end
      break unless found
    end
    after(1) { verbose('ping'); dispatch(schedule) } if schedule
  end

So you are essentially asking each ready/available processor to handle work from a randomly selected queue from the weighted @queues array. Manager#find_work would just take the queue instead of the index. This way if the @queues happens to be large based on someone setting high weights, you are only iterating over the number of processors, not the length of @queues. This also results in less calls to redis.

Thoughts?

@mperham
Owner
mperham commented Feb 11, 2012

I like it, great idea.

@ryanlecompte
Contributor

Cool! Are you okay with me making the changes this weekend for this?

@mperham
Owner
mperham commented Feb 11, 2012

Please do.

@ryanlecompte
Contributor

Thanks, will do.

@ryanlecompte
Contributor

I went ahead and made the changes now, commits are here: f400b4b
701edac

@mperham
Owner
mperham commented Feb 11, 2012

So much cleaner, awesome work!

@mperham mperham closed this Feb 11, 2012
@ryanlecompte
Contributor

Thanks! I'll review your redis stats work later this weekend, first glance looks awesome though!

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