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

Add option to AMQP transports to bind the queue to the exchange #1633

Merged
merged 1 commit into from Dec 15, 2015

Conversation

Projects
None yet
2 participants
@bernd
Member

bernd commented Dec 15, 2015

This was missing before and the exchange and routing-key options were not used.

The binding option defaults to "false" to avoid existing setups to fail when the given exchange (there is a default) does not exist.

Fixes #1599

@bernd bernd added this to the 1.3.1 milestone Dec 15, 2015

@@ -292,6 +294,15 @@ public ConfigurationRequest getRequestedConfiguration() {
);

cr.addField(
new BooleanField(
CK_EXCHANGE_BIND,
"Bind to exchange?",

This comment has been minimized.

@joschi

joschi Dec 15, 2015

Contributor

Is this a question? 😉

@@ -108,6 +110,9 @@ public void run() throws IOException {
for (int i = 0; i < parallelQueues; i++) {
final String queueName = String.format(queue, i);
channel.queueDeclare(queueName, true, false, false, null);
if (exchangeBind) {
channel.queueBind(queueName, exchange, routingKey);

This comment has been minimized.

@joschi

joschi Dec 15, 2015

Contributor

Should we check whether the exchange exists and issue a warning if it doesn't?

This comment has been minimized.

@bernd

bernd Dec 15, 2015

Member

If the input cannot be started due to a missing exchange, there is a warning in the web interface that is enough to debug the issue. It actually says that the exchange does not exist.

CK_EXCHANGE_BIND,
"Bind to exchange?",
false,
"Binds the queue to the configured exchange. Make sure the exchange exists!"

This comment has been minimized.

@joschi

joschi Dec 15, 2015

Contributor

Instead of "Make sure the exchange exists!" maybe write "The exchange must already exist."

Add option to AMQP transports to bind the queue to the exchange
This was missing before and the exchange and routing-key options were
not used.

The binding option defaults to "false" to avoid existing setups to fail
when the given exchange (there is a default) does not exist.

Fixes #1599

@bernd bernd force-pushed the issue-1599 branch from 7f3db18 to 811a232 Dec 15, 2015

@bernd

This comment has been minimized.

Member

bernd commented Dec 15, 2015

I updated the wording for the config options.

@joschi

This comment has been minimized.

Contributor

joschi commented Dec 15, 2015

LGTM. 👍

joschi added a commit that referenced this pull request Dec 15, 2015

Merge pull request #1633 from Graylog2/issue-1599
Add option to AMQP transports to bind the queue to the exchange

@joschi joschi merged commit ded6a5a into 1.3 Dec 15, 2015

0 of 3 checks passed

ci Jenkins build graylog2-server-integration-pr 455 has failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@joschi joschi deleted the issue-1599 branch Dec 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment