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

Add delay option into Shoryuken.add_group #543

Conversation

pizza-planet
Copy link

@pizza-planet pizza-planet commented Dec 12, 2018

@phstc
Copy link
Collaborator

phstc commented Dec 19, 2018

@lucas-ibotta yay that's great! Thank you. Sorry for the delay to get back to you, I will have a look to this this week and merge it down!

@pizza-planet
Copy link
Author

Sounds great. Let me know about lib/shoryuken/environment_loader.rb. I think I'm missing where it parses the YML variables to add the delay into the group.

@pizza-planet
Copy link
Author

pizza-planet commented Dec 21, 2018

There's two blockers on this currently:

  • Code climate checks that seems ignorable. What do you think about those?
  • The tests are passing for me locally with rspec but bundle exec rspec raises a user-defined signal 1 that causes the build to exit with a 1. That's happening on Travis also. Seems like an issue with bundler.

Any insight into these issues?

EDIT: found the issue with the delay tests and refactored it a bit. bundle exec rspec is passing locally for me now.

@pizza-planet
Copy link
Author

Cool. Tests are passing now. Just code climate remains.

@phstc
Copy link
Collaborator

phstc commented Dec 22, 2018

@lucas-ibotta thanks. I will test it locally to see how it goes. Don't worry about these two feedback from Code Climate - it is fine.

Copy link
Collaborator

@phstc phstc left a comment

Choose a reason for hiding this comment

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

@lucas-ibotta it is looking good, I just left a few minor comments, can you 👀?

BTW did you have a chance to test it locally?

lib/shoryuken/polling/base.rb Outdated Show resolved Hide resolved
.byebug_history Outdated Show resolved Hide resolved
lib/shoryuken/polling/strict_priority.rb Outdated Show resolved Hide resolved
@pizza-planet
Copy link
Author

Yes, nice catch. I have been on holiday, but I'll address those points soon and test things out locally. 👌

lib/shoryuken/options.rb Outdated Show resolved Hide resolved
@@ -57,10 +57,8 @@ def ==(other)
end
end

private
Copy link
Author

Choose a reason for hiding this comment

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

I made delay public and wrote some tests for it on each polling strategy.

@pizza-planet
Copy link
Author

Testing this locally and working out some of the bugs.

it 'sets delay based on group' do
delay_polling = Shoryuken::Polling::StrictPriority.new(queues, 25)
expect(delay_polling.send(:delay)).to eq(25.0)
expect(subject.send(:delay)).to eq(1.0)
Copy link
Author

Choose a reason for hiding this comment

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

I can't track down where this is a 1.0 instead of a 0.0.

@pizza-planet
Copy link
Author

pizza-planet commented Dec 31, 2018

Ok, I tested this locally with

# in setup_shoryuken.rb
Shoryuken.configure_server do |config|
  Shoryuken.add_group('no-delay', 5)
  Shoryuken.add_queue("https://sqs.us-east-1.amazonaws.com/../normal-queue", 5, 'no-delay')
  Shoryuken.add_group('delay', 5, delay: 25)
  Shoryuken.add_queue("https://sqs.us-east-1.amazonaws.com/../delay-queue", 5, 'delay')
end

class DelayWorker
  include Shoryuken::Worker
    shoryuken_options queue: 'https://sqs.us-east-1.amazonaws.com/../delay-queue'
                  
  def perform(_sqs_message, body)
    puts "DelayWorker Called with #{body}"
  end
end

class NormalWorker
    include Shoryuken::Worker
    shoryuken_options queue: 'https://sqs.us-east-1.amazonaws.com/../normal-queue'
                  
  def perform(_sqs_message, body)
    puts "NormalWorker Called with #{body}"
  end
end

Then ran bundle exec shoryuken --require ./setup_shoryuken.rb and everything worked!

You should probably also test from this branch to confirm?
There's another new code climate issue about delay that I'm looking into.

@phstc
Copy link
Collaborator

phstc commented Jan 2, 2019

@lucas-ibotta great work, thank you! I will have a look/test and get back to you. I may need a couple of days though, but if all is good I will merge it down and release a new version 🍻

@pizza-planet
Copy link
Author

Excellent. Thanks! Happy New Year to you too.

@phstc phstc merged commit 6f97035 into ruby-shoryuken:master Jan 6, 2019
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