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 cluster support to RabbitMQ transport #1118

Closed
wants to merge 28 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@thirkcircus
Copy link
Contributor

thirkcircus commented Apr 16, 2013

resolves issue #972

I'm reusing the https://github.com/mikehadlow/EasyNetQ implementation (see forum discussion: http://tech.groups.yahoo.com/group/nservicebus/message/18588)

EasyNetQ code copied in exactly as-is, same namespaces and everything, so bringing in any future improvements to EasyNetQ should be as easy as copy/paste. (EasyNetQ license is MIT, license also copied into the NSB project.)

The core NServiceBus implementation is now using the EasyNetQ connectionstring parser - I've modified the RabbitMqTransportConfigurer to use the cluster-aware connection factory bits:

var connectionStringParser = new ConnectionStringParser();
var connectionConfiguration = connectionStringParser.Parse(connectionString);
config.Configurer.ConfigureComponent<IConnectionConfiguration>(() => connectionConfiguration, DependencyLifecycle.SingleInstance);
config.Configurer.ConfigureComponent<IClusterHostSelectionStrategy<ConnectionFactoryInfo>>(x => new DefaultClusterHostSelectionStrategy<ConnectionFactoryInfo>(), DependencyLifecycle.InstancePerCall);
config.Configurer.ConfigureComponent<IConnectionFactory>(x => new ConnectionFactoryWrapper(
                                                                         x.Build<IConnectionConfiguration>(),
                                                                         x.Build<IClusterHostSelectionStrategy<ConnectionFactoryInfo>>()), DependencyLifecycle.InstancePerCall);
var connectionFactory = NServiceBus.Configure.Instance.Builder.Build < IConnectionFactory>();

I've also created some integration tests to prove the failover - these stand up a (new) cluster of local rabbitMQ nodes, run the test and take the cluster down again. See the readme in the cluster tests dir for requirements (although, assuming rabbit is installed then the tests should just work). Please let me know if there are any aspects of the tests which do fall over if/when someone else runs them - I've tried to make them machine-independent but there may be things I've missed.

example log output from the test "when_connected_to_local_cluster_and_nodes_start_going_down":
17-04-2013 9-01-53 a-m-

One request:
Internally we (my employer) are using daily NSB nuget packages off the develop branch - means we don't need to stand up our own NSB fork/nuget repo. But... this obviously means we don't get the benefits of our pull requests until they make it into develop. So... given I convinced my employer it was worth giving me a day to do this PR I'm now keen to see it progressed into develop sooner rather than later. Please. :)

@ghost ghost assigned andreasohlund Apr 17, 2013

@andreasohlund

This comment has been minimized.

Does this have to be a EasyQ.net type? (I want to contain this as much as possible)

@andreasohlund

This comment has been minimized.

The nsb parser has a few more fields that it parses. Do we have to use the EasyQ one?

@andreasohlund

This comment has been minimized.

can't we cast this to a IConnection?

@andreasohlund

This comment has been minimized.

Copy link
Member

andreasohlund commented Apr 17, 2013

To follow the direction we're taking with the other transports I would like the connections to be specifed like

<add name="NServiceBus/Transport" connectionString="host=defaultclusternode"/>

<add name="NServiceBus/Transport/Node2" connectionString="host=clusternode2"/>

<add name="NServiceBus/Transport/Node3" connectionString="host=clusternode3"/>

Would this be possible?

@andreasohlund

This comment has been minimized.

Copy link
Member

andreasohlund commented Apr 17, 2013

If you need this asap you can always plug this in using the existing extension point until we can pull this in:

https://github.com/NServiceBus/NServiceBus/blob/develop/src/RabbitMQ/NServiceBus.RabbitMQ/Config/RabbitMqTransportConfigurer.cs#L20

@thirkcircus

This comment has been minimized.

Copy link
Contributor

thirkcircus commented Apr 18, 2013

hi - so you're either working a really long day or you finished late and started early!
thanks for the quick look. :)

If you need this asap

yeah, we're trying to stick with the daily nuget pkgs from develop branch - so we don't have to muck around with our own internal repo and building, serving nuget pkgs etc. That's ok if the PR doesn't happen straight away, I just wouldn't want it to hang around for ages like some of the others that i've seen.

To follow the direction we're taking with the other transports...

how would this work with load balancing? It's nice having a single xml node to define the connection string because I can a) just reuse the EasyNetQ connection parser (there are a whole pile of tests to support this too) and b) switching between specifying the actual cluster nodes or just a load balancer address is very simple - just replace "host=defaultclusternode,clusternode2,clusternode3" with "host=loadbalanceraddress".
Even if each node had its own connectionstring, because there will be a load balancer in front of the cluster i'll likely have just a single connectionstring anyway...
The way rabbitmq has active-active clustering, if you specify separate connection strings (one per node) then you'll only ever hit the first defined node - until it is unavailable. A load balancer (and hence a single host in the connection string) will be necessary to utilise all the active nodes.
I've also looked at ConfigureTransport.Configure - at first look it appears to me that changing the configuration to support multiple connection strings for a single transport will be not a small change... whereas supporting multiple hosts in a single connection string already just works.

can't we cast this to a IConnection?
and: Does this have to be a EasyQ.net type? (I want to contain this as much as possible)

Yes, yes we can. :) Have done this. PersistentConnection now implements IConnection via delegation to the private connection.

The nsb parser has a few more fields that it parses. Do we have to use the EasyQ one?

oh, you're right, sorry, i missed those additional fields.
I do like the easynetq parser because it's very easy to add new things to the connectionstring definition and because it gives nice parsing errors (http://mikehadlow.blogspot.co.nz/2012/09/parsing-connection-string-with-sprache.html). I also like the abstraction of ConnectionConfiguration - the parser is responsible for determining the configuration - which is passed to the connectionfactory (rather than the parser creating the connectionfactory).
so, I have:

  1. copied in the easynetq connectionstringparsing tests (should have done that anyway)
  2. altered the existing NSB connection string tests to use the new parser.
  3. rather than copying the EasyNetQ ConnectionStringGrammar and Parser classes as-is, I have copied these into the NSB part of the project so I can control the connection string grammar.
  4. added retryDelay, maxRetries, usePublisherConfirms and maxWaitTimeForConfirms to ConnectionStringGrammar and ConnectionConfiguration
  5. passed the ConnectionConfiguration into the RabbitMqConnectionManager (rather than RetrySettings)
  6. added some more unit tests for the connection string parsing

Also, a change I've made to the defaults (in ConnectionConfiguration):

  • I've changed the default port from -1 to the rabbitmq default (5672). The original unit test specified -1 but i'm not sure why...?

Turned out to be a reasonable number of changes to include those additional config properties - tidied up the code a bit though.

@andreasohlund

This comment has been minimized.

Copy link
Member

andreasohlund commented Apr 18, 2013

I'd like to copy as little as possible from EasyQ and the only part (after the last changes) that I'm not convinced to use is the whole parsing thing. We stay away from external deps/libs/copy&paste like the plague. Do we really need to use Sprache? How much work would it be to get rid of it? (string split the connection string like we did before)

@andreasohlund

This comment has been minimized.

Copy link
Member

andreasohlund commented Apr 18, 2013

Perhaps like one of the commenters suggested in that blogpost?
http://pastie.org/4856625

@thirkcircus

This comment has been minimized.

Copy link
Contributor

thirkcircus commented Apr 18, 2013

ok, i can have a look at that.
i'd still definitely want to keep the connectionconfiguration class (separate from the parser) since I think that provides good separation of concerns but I can look at pulling out the use of sprache if you really don't want it. I only pulled it in to keep the (my) work to a minimum. :)

@andreasohlund

This comment has been minimized.

Copy link
Member

andreasohlund commented Apr 18, 2013

i'd still definitely want to keep the connectionconfiguration class
Agreed I like that as well!

On Thu, Apr 18, 2013 at 9:30 AM, Justin Thirkell
notifications@github.comwrote:

ok, i can have a look at that.
i'd still definitely want to keep the connectionconfiguration class
(separate from the parser) since I think that provides good separation of
concerns but I can look at pulling out the use of sprache if you really
don't want it. I only pulled it in to keep the (my) work to a minimum. :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/1118#issuecomment-16561611
.

http://andreasohlund.net
http://twitter.com/andreasohlund

@thirkcircus

This comment has been minimized.

Copy link
Contributor

thirkcircus commented Apr 19, 2013

hi, ok that's done.
and i can see your point about the use of sprache. agree, it was a fairly heavy-handed approach. 😀

@thirkcircus

This comment has been minimized.

Copy link
Contributor

thirkcircus commented Apr 19, 2013

and i've just looked at that pastie example again. and pushed that in as well.

@andreasohlund

This comment has been minimized.

Copy link
Member

andreasohlund commented Apr 19, 2013

Nice I'll review and pull in the next few days.
#1125 will most likely be
ahead of this one so I might ask you to do a rebase of you pull if there
are any conflicts.

Great work!

On Fri, Apr 19, 2013 at 4:08 AM, Justin Thirkell
notifications@github.comwrote:

and i've just looked at that pastie example again. and pushed that in as
well.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1118#issuecomment-16623958
.

http://andreasohlund.net
http://twitter.com/andreasohlund

thirkcircus added some commits Apr 21, 2013

Merge branch 'develop' of git://github.com/NServiceBus/NServiceBus in…
…to rabbitmq_cluster

Conflicts:
	src/RabbitMQ/NServiceBus.RabbitMQ.Tests/RabbitMqContext.cs
	src/RabbitMQ/NServiceBus.RabbitMQ/Config/RabbitMqTransportConfigurer.cs
@thirkcircus

This comment has been minimized.

Copy link
Contributor

thirkcircus commented Apr 21, 2013

ok, have merged in changes from 2c7f965
all looks ok. :)

@thirkcircus

This comment has been minimized.

Copy link
Contributor

thirkcircus commented Apr 21, 2013

ping @andreasohlund...

andreasohlund added a commit that referenced this pull request Apr 23, 2013

@andreasohlund

This comment has been minimized.

Copy link
Member

andreasohlund commented Apr 23, 2013

Pulled in 896290a

@thirkcircus thirkcircus deleted the thirkcircus:rabbitmq_cluster branch Apr 29, 2013

@thirkcircus thirkcircus restored the thirkcircus:rabbitmq_cluster branch Apr 29, 2013

johannesg pushed a commit to johannesg/NServiceBus that referenced this pull request Feb 4, 2014

Added clustering support to the RabbitMq connection manager
updated fody

updated R# dotsettings (dotcover filter settings)

added EasyNetQ nuget pkg reference

Revert "added EasyNetQ nuget pkg reference"

This reverts commit f821e60.

added r# dotsettings file to test project for naming styles

copied in EasyNetQ implementation of cluster-aware rabbitmq connectionmanager.

changed connectionmanager to use easynetq persistent (retrying and cluster-config-aware) connections

altered tests to work with new easynetq-derived connections

added test for clustered nodes.

removed connection-per-purpose for the moment

clustering node-down test passing

completed clustering integration test

tidied up integration tests

split multiple-types-per-file into separate files

overriding easynetq default client properties

return IConnection rather than IPersistentConnection

changes as per Particular#1118 (comment)

make integration tests explicit

add ncrunch temp dir to ignore

removed use of sprache, simple parser is back.

tidied up the unit tests and rabbit config files

multiple host parsing is a little less ugly

matching connection string parts to ConnectionConfiguration properties by convention

fix typo

remove old comment

update configuration to use new connectionstring parser
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment