Skip to content

Simplify BasicFetch#queues_cmd#618

Merged
mperham merged 1 commit intosidekiq:masterfrom
brainopia:patch-1
Jan 12, 2013
Merged

Simplify BasicFetch#queues_cmd#618
mperham merged 1 commit intosidekiq:masterfrom
brainopia:patch-1

Conversation

@brainopia
Copy link

And improve weight honoring.

And improve weight honoring.
@mperham
Copy link
Collaborator

mperham commented Jan 11, 2013

I hate changing this method because it's hard to evaluate whether we would see proper weighting. It looks ok to me. Anyone else confirm/deny?

@brainopia
Copy link
Author

Ok, I've made a simple program to demonstrate deficiencies of current approach and how new one fixes it.

Lets test using following weights: a: 3, b: 3, c: 1, d: 1, e: 1, f: 1

So the ``c, d, e, f` should have the same distribution, right? In current approach they are not, they have totally different distribution for last positions of queue list. Let me demonstrate it.

The program is (you don't have to read it, just look at results at the end )

RUN_NUMBER = 100_000

def weights(data)
  RUN_NUMBER.times
    .map { yield data }.transpose
    .map do |results_for_position|
      results_for_position
        .group_by {|it| it }
        .map {|key, values| [key, values.size] }
    end
end

def weights1(data)
  weights(data) do |it|
    sampled = it.sample(it.uniq.size).uniq
    sampled.concat(it.uniq - sampled)
  end
end

def weights2(data)
  weights(data) {|it| it.shuffle.uniq }
end

def percents(all_weights)
  all_weights.each.with_index do |position_weights, index|
    puts "-- #{index + 1} position"
    position_weights.sort_by(&:first).each do |item, weight|
      puts "---- item #{item} has #{(weight.to_f / RUN_NUMBER * 100).to_i}%"
    end
  end
end

def test(data)
  puts 'previous weights'
  percents weights1(data)
  puts
  puts 'proposed weights'
  percents weights2(data)
end

test [:a, :a, :a, :b, :b, :b, :c, :d, :e, :f]

___END___

previous weights
-- 1 position
---- item a has 30%
---- item b has 29%
---- item c has 10%
---- item d has 10%
---- item e has 9%
---- item f has 9%
-- 2 position
---- item a has 26%
---- item b has 26%
---- item c has 11%
---- item d has 11%
---- item e has 12%
---- item f has 11%
-- 3 position
---- item a has 20%
---- item b has 19%
---- item c has 15%
---- item d has 14%
---- item e has 14%
---- item f has 14%
-- 4 position
---- item a has 13%
---- item b has 13%
---- item c has 23%
---- item d has 18%
---- item e has 15%
---- item f has 15%
-- 5 position
---- item a has 7%
---- item b has 7%
---- item c has 28%
---- item d has 27%
---- item e has 20%
---- item f has 7%
-- 6 position
---- item a has 2%
---- item b has 2%
---- item c has 10%
---- item d has 17%
---- item e has 27%
---- item f has 40%

proposed weights
-- 1 position
---- item a has 29%
---- item b has 30%
---- item c has 10%
---- item d has 10%
---- item e has 9%
---- item f has 10%
-- 2 position
---- item a has 26%
---- item b has 26%
---- item c has 11%
---- item d has 11%
---- item e has 11%
---- item f has 11%
-- 3 position
---- item a has 20%
---- item b has 20%
---- item c has 14%
---- item d has 14%
---- item e has 14%
---- item f has 14%
-- 4 position
---- item a has 13%
---- item b has 13%
---- item c has 18%
---- item d has 18%
---- item e has 18%
---- item f has 18%
-- 5 position
---- item a has 7%
---- item b has 7%
---- item c has 21%
---- item d has 21%
---- item e has 21%
---- item f has 21%
-- 6 position
---- item a has 2%
---- item b has 2%
---- item c has 23%
---- item d has 23%
---- item e has 23%
---- item f has 23%

If you look at 4, 5 and 6 position distributions, then you'll see that the current approach gives non-uniform distribution of items (because you've used concat @uniq_queues - queues instead of concat((@uniq_queues - queues).shuffle))

And new approach while being simpler works as advertised.

mperham added a commit that referenced this pull request Jan 12, 2013
Simplify BasicFetch#queues_cmd
@mperham mperham merged commit dc8bd93 into sidekiq:master Jan 12, 2013
@brainopia brainopia deleted the patch-1 branch January 12, 2013 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants