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

Documentation for setting Receive Message options are confusing/unexpected when used with groups #735

Closed rajyan closed 1 year ago

rajyan commented 1 year ago

https://github.com/ruby-shoryuken/shoryuken/wiki/Receive-Message-options https://github.com/ruby-shoryuken/shoryuken/wiki/Long-Polling

We are using Shoryuken with multiple groups.

When setting long polling like in the docs

Shoryuken.sqs_client_receive_message_opts = { 
  wait_time_seconds: 20, 
  max_number_of_messages: 1 
}

it looks as if the client option is applied to all groups, but we have noticed that it is only applied to the 'default' group.

source: https://github.com/ruby-shoryuken/shoryuken/blob/f24db5422ef6869c4a556c134a27b4259027e7b8/lib/shoryuken/options.rb#L82 https://github.com/ruby-shoryuken/shoryuken/blob/f24db5422ef6869c4a556c134a27b4259027e7b8/lib/shoryuken/fetcher.rb#L76-L81

This is a confusing behavior, because some other worker options like delay defaults with the default option if not specified per group.

source: https://github.com/ruby-shoryuken/shoryuken/blob/f24db5422ef6869c4a556c134a27b4259027e7b8/lib/shoryuken/options.rb#L39-L40 https://github.com/ruby-shoryuken/shoryuken/blob/f24db5422ef6869c4a556c134a27b4259027e7b8/lib/shoryuken/options.rb#L73-L75

I would prefer that the behavior of the options be consistent across all options (but fixing in this way is a breaking change in some cases), or at least this behavior should be documented.

phstc commented 1 year ago

Hi @rajyan

I believe Shoryuken.sqs_client_receive_message_opts was introduced before groups. This setter Shoryuken.sqs_client_receive_message_opts= was kept for backward compatibility.

I would prefer that the behavior of the options be consistent across all options

That also not be obvious to all users and probably requires an update in the docs to make it more explicit.

Another alternative would be to update the two docs you linked

Explaining that Shoryuken.sqs_client_receive_message_opts= is only for default and for groups, you must set Shoryuken.sqs_client_receive_message_opts[group-name]=

Potentially add a logger.warn here saying that setting props like that only applies to the default group. But this can be a bit noisy.

rajyan commented 1 year ago

@phstc Thank you for you quick response and this great library!

That also not be obvious to all users and probably requires an update in the docs to make it more explicit.

Thank you for your opinions. Fair enough, I would try to update the docs then.

I updated the wiki to try to make them more explicit about the behavior for groups. https://github.com/ruby-shoryuken/shoryuken/wiki/Long-Polling/_compare/8e350bba4284578fbac36f154b2689fc351d4855...878afc92a056fb43b62c1df7b2948a21783817e2 https://github.com/ruby-shoryuken/shoryuken/wiki/Receive-Message-options/_compare/5c48110f75c9165608b2d655ce30f94fc66ee1fa...ba00949d0a31feb2f0ddbb5b1eb9c8a6e1814fc5

phstc commented 1 year ago

The updates look great. Thank you for adding them!

rajyan commented 1 year ago

Thank you!