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

Look for polling strategy inside groups config #417

Merged
merged 6 commits into from
Jul 31, 2017

Conversation

atyndall
Copy link
Contributor

@atyndall atyndall commented Jul 25, 2017

Allows group config to specify polling strategy in the .yml like so;

timeout: 1140
delay: 60

groups:
  r:
    concurrency: 1
    polling_strategy: StrictPriority
    queues:
      - x
      - y
      - z
  q:
    concurrency: 1
    polling_strategy: StrictPriority
    queues:
      - a
      - b
      - c

or like so

timeout: 1140
delay: 60
concurrency: 1
polling_strategy: StrictPriority
queues:
  - x
  - y
  - z

Note: this change would be backwards incompatible with the current way of setting strategy described here. I have a version that also preserves that behaviour, but it's more complicated.

options[group].to_h.fetch(:polling_strategy, Polling::WeightedRoundRobin)
option_group = group == 'default' ? options : options[:groups][group]
strategy = option_group.to_h.fetch(:polling_strategy, Polling::WeightedRoundRobin)
strategy = "Shoryuken::Polling::#{strategy}".constantize if strategy.is_a?(String)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atyndall constantize depends on ActiveSupport.

Maybe the simplest way of doing it would be:

if strategy == 'StrictPriority'
  Polling::StrictPriority
else
  Polling::WeightedRoundRobin
end

WDYT?

option_group = group == 'default' ? options : options[:groups][group]
strategy = option_group.to_h.fetch(:polling_strategy, Polling::WeightedRoundRobin)
strategy = "Shoryuken::Polling::#{strategy}".constantize if strategy.is_a?(String)
strategy
end

def start_callback
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atyndall WDYT about adding some test cases?

# options_spec.rb

describe '.polling_strategy' do
  context 'when not set' do
    specify do
      expect(Shoryuken.polling_strategy('default')).to eq Shoryuken::Polling::WeightedRoundRobin
    end
  end

  context 'when set' do
    before do
      Shoryuken.options[:groups] = {
        'group1' => {
          polling_strategy: 'StrictPriority'
        }
      }
    end

    specify do
      expect(Shoryuken.polling_strategy('default')).to eq Shoryuken::Polling::WeightedRoundRobin
      expect(Shoryuken.polling_strategy('group1')).to eq Shoryuken::Polling::StrictPriority
    end
  end
end

@atyndall
Copy link
Contributor Author

@phstc Made changes as you suggested. I also restructured it a bit to ensure that if a user mistypes a string, it errors instead of silently picking the WeightedRoundRobin strategy.

@phstc phstc merged commit c753f0c into ruby-shoryuken:master Jul 31, 2017
@phstc
Copy link
Collaborator

phstc commented Jul 31, 2017

@atyndall yay added to master 🍻 I will ship a new release tomorrow including your changes.

@phstc
Copy link
Collaborator

phstc commented Jul 31, 2017

@atyndall 3.1.7 is out with your change 🍻

ota42y added a commit to ota42y/rising_dragon that referenced this pull request Aug 14, 2017
shoryuken broken change for group.
ruby-shoryuken/shoryuken#417

but, "default" group is not affected, so change it.
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