ruby-shoryuken / shoryuken

A super efficient Amazon SQS thread based message processor for Ruby. This project is in MAINTENANCE MODE. Reach me out on Slack link on the description if you want to become a new maintainer.
Other
2.06k stars 280 forks source link

Add default body value to worker perform methods #733

Closed AlexandreL0pes closed 1 year ago

AlexandreL0pes commented 1 year ago

Context

Recently, I’ve been working on a new shoryuken worker and this particulary one does not require an argument to be scheduled and executed.

For this scenario, the solution was give a empty string as the argument, since the body argument it’s required, something like this:

class Job
  include Shoryuken::Worker

  shoryuken_options queue: "queue-name", auto_delete: true

  def perform(_message, _body)
  end
end

Job.perform_async(" ")

Suggestion

The idea with this PR is turn the body argument into optional, passing a string with a blank space as the default value, because it’s not possible to send a message without body, according to the aws sqs client (link).

With the changes, workers could be called like:

class Job
  include Shoryuken::Worker

  shoryuken_options queue: "queue-name", auto_delete: true

  def perform(_message, _body)
  end
end

Job.perform_async

Let me know if this makes sense, happy to close if not!

phstc commented 1 year ago

@AlexandreL0pes @andreleoni Olá! Shoryuken tries to be more transparent with SQS, and SQS does not accept empty bodies. A change like this may require a major version bump to avoid breaking changes.

Are you migrating an existing code base with Sidekiq (empty body) to Shoryuken, or is it more a dev experience suggestion?

AlexandreL0pes commented 1 year ago

Olá, @phstc!! It's a dev experience suggestion.

This is a new worker, created since some latency issues were identified, so this slow method has been converted into a shoryuken worker.

phstc commented 1 year ago

@AlexandreL0pes got it. Thinking more about this, I believe it might be better to add an entry on the migration guide.

SQS does not support empty messages. With this PR, Job.perform_async will actually send a message with spaces, and that might be unexpected.

Whereas with Sidekiq, I believe you get a real empty message.

AlexandreL0pes commented 1 year ago

Okay, I'm closing this PR and later I'll add this topic to the migration guide.

Thanks for the clarifications and congratulations for this awesome project!