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

LOGSTASH-691: move amqp plugin to be an external plugin #338

Merged
merged 32 commits into from Feb 12, 2013
Merged

LOGSTASH-691: move amqp plugin to be an external plugin #338

merged 32 commits into from Feb 12, 2013

Conversation

zaccari
Copy link
Contributor

@zaccari zaccari commented Jan 29, 2013

This renames the amqp input/output plugins to rabbitmq, with updates to documentation and references for the elasticsearch-river & zenoss plugins.

# other amqp broker will not work with this plugin. I do not know why. If you
# need support for brokers other than rabbitmq, please file bugs here:
# <https://github.com/ruby-amqp/bunny> </b>
# Pull events from a RabbitMQ exchange.
Copy link
Contributor

Choose a reason for hiding this comment

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

If you have a recommended version, can you specify it? Many distros ship with ancient versions of rabbitmq that probably don't work anymore with the bunny gem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the input/output plugins with version info for Bunny & RabbitMQ, and also added version requirements to the gemspec for Bunny 0.8.x. Let me know if any more information would help.

@jordansissel
Copy link
Contributor

Going to try and reach out to some existing logstash amqp users to help review this since I've forgotten everything about amqp.


# Your amqp broker's custom arguments. For mirrored queues in RabbitMQ: [ "x-ha-policy", "all" ]
# Custom arguments. For mirrored queues in rabbitmq: [ "x-ha-policy", "all" ]
Copy link

Choose a reason for hiding this comment

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

With RabbitMQ 3x, we do not need any more to user x-ha-policy when declaring then queue.
Mirroring is done by policy, so the customs arguments are not needed for HA.
See http://www.rabbitmq.com/blog/2012/11/19/breaking-things-with-rabbitmq-3-0/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the custom arguments variable no longer needed (and should be removed), or should just the doc example with policy be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes rabbitmq 3. Ubuntu 12.04 still ships RabbitMQ 2.7, so I expect there to continue to be ubuntu users on 2.x at least another 2 years and debian for another 4-6 years.

Copy link

Choose a reason for hiding this comment

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

This is a good point.

So it could be good to have both configuration extract for RabbitMQ 2x and
3x.

@@ -36,7 +36,7 @@ Gem::Specification.new do |gem|
gem.add_runtime_dependency "aws-sdk"
gem.add_runtime_dependency "heroku"
gem.add_runtime_dependency "addressable", ["~> 2.2.6"]
gem.add_runtime_dependency "bunny"
gem.add_runtime_dependency "bunny", ["~> 0.8.0"]
Copy link

Choose a reason for hiding this comment

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

Since logstash release are quite frequent, we should watch the release of the stable 0.9 version of bunny.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bumped bunny to latest version here: 4867a89.

More information about the latest version of bunny can be found
here: https://github.com/ruby-amqp/bunny#changes-in-bunny-09
frame_max is the maximum permissible size of a frame (in bytes)
to negotiate with clients.
Until we have a way to know which version of rabbitmq the
use has, we should let them choose whether to use the
arguments variable or not.
@jordansissel
Copy link
Contributor

@mzaccari - do you feel this is ready for merge?

@jordansissel
Copy link
Contributor

Gonna merge.

Post-merge tasks:

  • make an 'amqp' input and output plugin that subclasses the new one, flag it deprecated so existing users are directed the right way.

jordansissel added a commit that referenced this pull request Feb 12, 2013
LOGSTASH-691: move amqp plugin to be an external plugin
@jordansissel jordansissel merged commit 3146d3c into elastic:master Feb 12, 2013
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.

None yet

5 participants