Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make using sets for users and groups optional #104

Merged
merged 1 commit into from Sep 28, 2016

Conversation

burin
Copy link
Contributor

@burin burin commented Sep 22, 2016

PR to address #103

This PR makes using sets optional:

Rollout.new(Redis.new, use_sets: true) will use the old behavior with sets

Rollout.new(Redis.new) will use the "new" behavior with arrays. This would be the new default. If you prefer to use set as default, I can open a new PR and make the "new" behavior optional.

require 'redis'
rollout_client = Rollout.new(Redis.new)
iterations = 100
rollout_client.activate(:test_feature)
raw_users = []
100_000.times {|num| raw_users << num }

rollout_client_sets = Rollout.new(Redis.new, use_sets: true)
rollout_client_sets.activate_users(:test_feature, raw_users)

Benchmark.bm do |bm|
  bm.report do
    iterations.times do
      rollout_client.active?(:test_feature, '123')
    end
  end

  bm.report do
    iterations.times do
      rollout_client_sets.active?(:test_feature, '123')    end
  end

end

### 3.650000   0.200000   3.850000 (  3.897240)
### 13.150000   0.250000  13.400000 ( 13.426378)

@burin
Copy link
Contributor Author

burin commented Sep 28, 2016

@reneklacan any feedback on this PR? In particular the default behavior: should it use sets by default or arrays? (this PR changes default behavior)

@reneklacan
Copy link
Member

@burin sorry, I missed PR submission, it looks all good 👍

@reneklacan reneklacan merged commit 11c5d33 into fetlife:master Sep 28, 2016
@burin
Copy link
Contributor Author

burin commented Sep 29, 2016

thanks @reneklacan for the quick turnaround

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.

None yet

2 participants