add events for connection recovery errors and connection success.

[kjnilsson]

Fixes #156

Raised against master as it extends IConnection as well as making more sense with auto recovery as the default.

ConnectionRecoveryError is raised after each failed connection recovery attempt.

ConnectionRecoverySucceeded is raise once after all the recovery has completed.

[bording]

@kjnilsson Seeing how this is going into master which is now following SemVer, are you planning on bumping to 5.0 for this? Modifying an interface is a breaking change.

[michaelklishin]

@bording we will bump to 4.2.0.

[bording]

@bording we will bump to 4.2.0.

@michaelklishin IConnection is part of your public API. Modifying an interface in any way, including adding members, is both a compile-time and binary breaking change. SemVer dictates that this needs to be a 5.0 release.

[michaelklishin]

@bording that's not my understanding of SemVer, sorry. 5.0 would be warranted by major breaking API changes. This change is nothing major. I don't think we want to go all Chrome with versioning, sorry.

[michaelklishin]

Alternatively we can go up to 5.0 and delay shipping this until there's enough new stuff to warrant a 5.0. This can take 6 to 12 months, or never happen because this client will go on life support in the 2nd half of next year.

That sounds like a situation that's way worse than using a more loose interpretation of SemVer. Our goal is not to be blind followers of SemVer. Our goal is to ship useful stuff reasonably quickly and make upgrades reasonably predictable. If this means deviating from SemVer a bit, I have no problem with that.

[kjnilsson]

I think it is fair to say that following strict SemVer didn't really turn out to be practical for this project at this stage. Saying that I would like to try to avoid breaking source level compatibility and although I can't see how anyone could usefully implement and alternative IConnection it may have been implemented in test code. Will have a think to see if there are any good alternative locations for these events to go although I think they make most sense to be located with the rest of the events.

[bording]

I think it is fair to say that following strict SemVer didn't really turn out to be practical for this project at this stage

I'm not sure I see why this has to be the case, but if that is true, then I'd hope you can publicize the fact that you aren't following SemVer after claiming that you would. Otherwise you are doing a disservice to the people using the library by introducing breaking changes without bumping the major version.

Saying that I would like to try to avoid breaking source level compatibility and although I can't see how anyone could usefully implement and alternative IConnection

The older (but still supported) 3.x releases of the NServiceBus RabbitMQ transport do implement IConnection, so this change is both a compile-time break for us, and a binary runtime break for any of our customers who might try to update to this new client.

[michaelklishin]

This is not going into 3.6.x. @bording I've outlined three scenarios above:

• Going nuts with major version bumps (a single method added warrants a new major version)
• Delaying major releases for 6-12 months
• Accepting a minor version bump as reasonable and shipping small breaking changes (in particular newly added API elements) quickly

which one would NServiceBus prefer to see in the 4.x series of this client?

[michaelklishin]

@bording to be 100% clear, I find option 1 above ridiculous, option 2 very inconvenient for both us and the community (people will be running preview versions in production just to get the tiny changes they need) and option 3 to be perfectly fine.

[michaelklishin]

Can't NServiceBus lock down the supported RabbitMQ client versions to a range? This is a common practice in several other communities (as different as Ruby, JS/Node, Erlang/Elixir). Even if you can't then you surely can document what RabbitMQ client versions are supported.

[bording]

which one would NServiceBus prefer to see in the 4.x series of this client?

Option 1 is the closest to what would be preferable, but I do want to point out that if you were just adding a single method to a class, then that isn't a breaking change, and would be perfectly fine to release as a minor bump. The main thing I've been trying to point out here is that making the same change to an interface is actually a breaking change, and would require a major bump according to SemVer.

Given that you had previously said that the 4.x would be following SemVer, my intention here was to help make that true! 😄

Regarding NServiceBus specifically, our 3.5.x (along with our latest 4.x versions) releases have moved up to the 4.x RabbitMQ client releases, so we aren't concerned by what's going on with your 3.6.x releases any more. We know those never claimed SemVer, so we dealt with that as needed.

In general, for dependencies that do follow SemVer, we usually lock our supported range to a specific major version, which would be safe to do and allow for the most flexibility.

For the RabbitMQ client, we lock our dependencies to a range as well, currently to within a specific minor release. Historically this was because of the 3.6.x line's lack of SemVer.

For the 4.x RabbitMQ client releases, I had been hoping to open up the range to the entire 4.x major release, but for now we'll just keep it more restricted, and consider restricting it further based on the conversation here.

[michaelklishin]

@bording OK, this makes sense. We will bump to 5.0.0. @kjnilsson has a few more 5.0-y things in mind :)

[michaelklishin]

@bording that said, I think pinning to a minor RabbitMQ client version range is a good idea for projects such as NServiceBus. Not all breaking changes are intentionally breaking, some bugs require breaking public API changes but shipping them cannot wait for 6 months for a release, and so on. So being more conservative about what dependency versions you support is not a bad strategy.