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 shoryuken async strategy #532

Merged
merged 11 commits into from
Jun 28, 2017
Merged

Add shoryuken async strategy #532

merged 11 commits into from
Jun 28, 2017

Conversation

josephchoe
Copy link

I added a new strategy for AWS SQS-based queues using the shoryuken gem.

@pyromaniac
Copy link
Contributor

Cool, can you also fix the readme and the changelog please?

@josephchoe
Copy link
Author

Done.

@pyromaniac
Copy link
Contributor

ok, can you add the changelog entry on top, please? Also, did you test it manually? Does it work? If so, I'll merge it as soon as specs pass

@josephchoe
Copy link
Author

Oops, sorry.

@josephchoe
Copy link
Author

The specs pass, and I've tested it manually. But I think I've noticed a problem in copying the worker and specs from the sidekiq strategy to shoryuken. Shoryuken only passes in sqs_msg and body into the worker and not options, which would be a problem for passing in suffix.

https://github.com/phstc/shoryuken/blob/master/lib/shoryuken/processor.rb#L17

@pyromaniac
Copy link
Contributor

can we use Shoryuken::Worker.perform_async({index: type, ids: ids, options: options}, queue: shoryuken_queue) ?

def leave
@stash.each do |type, ids|
next if ids.empty?
Shoryuken::Worker.perform_async({index: type, ids: ids}, queue: shoryuken_queue)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, how about to use {type: type ?

Copy link
Author

Choose a reason for hiding this comment

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

Will do.

@josephchoe
Copy link
Author

Made the requested changes. Let me know if there's anything else that needs to be addressed.

@pyromaniac pyromaniac merged commit abae6bc into toptal:master Jun 28, 2017
@pyromaniac
Copy link
Contributor

Cool, merged, thanks!

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