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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add delay option into Shoryuken.add_group #543

Merged

Conversation

@lucas-ibotta
Copy link
Contributor

lucas-ibotta commented Dec 12, 2018

  • 馃憢 Hey Pablo! Thanks for all your work on Shoryuken. 馃槃
  • Fixes #454
@phstc

This comment has been minimized.

Copy link
Owner

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!

@lucas-ibotta

This comment has been minimized.

Copy link
Contributor Author

lucas-ibotta commented Dec 19, 2018

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.

lib/shoryuken/polling/base.rb Outdated Show resolved Hide resolved
@lucas-ibotta

This comment has been minimized.

Copy link
Contributor Author

lucas-ibotta 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.

@lucas-ibotta

This comment has been minimized.

Copy link
Contributor Author

lucas-ibotta commented Dec 21, 2018

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

@phstc

This comment has been minimized.

Copy link
Owner

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
Owner

phstc left a comment

@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
@lucas-ibotta

This comment has been minimized.

Copy link
Contributor Author

lucas-ibotta commented Dec 28, 2018

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
lib/shoryuken/options.rb Outdated Show resolved Hide resolved
鈥cas-ibotta/shoryuken into feature/add_delay_into_group_interface
@@ -57,10 +57,8 @@ def ==(other)
end
end

private

This comment has been minimized.

Copy link
@lucas-ibotta

lucas-ibotta Dec 31, 2018

Author Contributor

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

@lucas-ibotta

This comment has been minimized.

Copy link
Contributor Author

lucas-ibotta commented Dec 31, 2018

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)

This comment has been minimized.

Copy link
@lucas-ibotta

lucas-ibotta Dec 31, 2018

Author Contributor

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

@lucas-ibotta

This comment has been minimized.

Copy link
Contributor Author

lucas-ibotta 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

This comment has been minimized.

Copy link
Owner

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 馃嵒

@lucas-ibotta

This comment has been minimized.

Copy link
Contributor Author

lucas-ibotta commented Jan 2, 2019

Excellent. Thanks! Happy New Year to you too.

@phstc phstc merged commit 6f97035 into phstc:master Jan 6, 2019
1 of 2 checks passed
1 of 2 checks passed
codeclimate 3 issues to fix
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.