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

Don't create AMQP exchange if not configured #3647

Closed
wants to merge 4 commits into from

Conversation

hanej
Copy link

@hanej hanej commented Jan 8, 2018

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Removed AMQP publisher specific code and documentation from the consumer plugin. This has been validated in our environment. Fixes #3646

@hanej hanej changed the title Remove Remove AMQP Publisher Code Jan 8, 2018
@hanej hanej changed the title Remove AMQP Publisher Code Remove AMQP Publisher Code for Consumer Jan 8, 2018
@danielnelson
Copy link
Contributor

If I understand this correctly, the user would now need to define the exchange and the queue, as well as bind them before using the plugin?

@hanej
Copy link
Author

hanej commented Jan 9, 2018

That is correct

@danielnelson
Copy link
Contributor

We will need to find a way to provide what you need without breaking backwards compatibility, perhaps we could modify the code to skip the creation of the exchange if exchange is unset and then declare the exchange and queue only if they do not exist?

@hanej
Copy link
Author

hanej commented Jan 9, 2018 via email

@hanej hanej changed the title Remove AMQP Publisher Code for Consumer Don't create AMQP exchange if not configured Jan 9, 2018
@hanej
Copy link
Author

hanej commented Jan 9, 2018

The change has been committed. It works with and without the exchange configuration option.

)
if err != nil {
return nil, fmt.Errorf("Failed to bind a queue: %s", err)
err = ch.QueueBind(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should always QueueBind, so that the binding key works even if no exchange/queue is created.

@danielnelson danielnelson added this to the 1.7.0 milestone May 25, 2018
@danielnelson danielnelson added area/rabbitmq feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels May 25, 2018
@danielnelson
Copy link
Contributor

It seems to me that this pull request is a work around for the limited configurability of the exchange declaration in the amqp plugin, and if we improved that there wouldn't be a need to skip declaring the exchange. I'm adding more configuration option around the creation of exchanges in #4228 and #3785, please let me know if the new options are sufficient.

@danielnelson danielnelson removed this from the 1.7.0 milestone Jun 5, 2018
@danielnelson
Copy link
Contributor

Closing, assuming this is no longer required. One related pull request is #5831 which adds passive queue declaration as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rabbitmq feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AMQP_Consumer Altering Exchanges
2 participants