-
Notifications
You must be signed in to change notification settings - Fork 137
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
Allow configuration of exchange options #170
Conversation
@@ -13,6 +13,7 @@ def self.initialize(params={}) | |||
mq_host: 'localhost', | |||
mq_port: 5672, | |||
mq_exchange: 'hutch', # TODO: should this be required? | |||
mq_exchange_options: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some unsolicited advice, I would make this default to { durable: true}
instead of blank.
That way, anyone who upgrades won't get any big surprises, as in, their queues aren't durable anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this since you are already doing the hash merge thing in broker.rb above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes more sense to me to move new default into the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that but then there might be people who directly set Hutch::Config.set :mq_exchange_options, custom: :thing
and then mistakenly lose the default durable option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it's a pain to have to remember to re-set what is a default option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a new functionality I think we can put a warning comment near this option. If somebody looks to find which is the required configuration key for this, will see that default is durable and also will see the comment.
Allow configuration of exchange options
Thank you. Sorry that has gone under my radar. |
No description provided.