Inform recovery listeners when recovery has started #132
Conversation
Source files are moved from `src` to `src/main/java`. Tests are moved
from `test` to `src/test/java`.
`build.properties` is moved and split into two files:
src/test/resources/build.properties
src/test/resources/config.properties
build.xml is replaced by pom.xml.
Required system properties (eg. `make.bin`) are now verified in
AbstractRMQTestSuite.java. Properties are verified before the tests
start. This avoids to have a testsuite hanging because `make` is not GNU
Make for instance.
Fixes #37.
Move build system to Maven (retains Ant tasks for backwards compatibility)
This fixes the build with Java 1.7.
This should fix `make distclean` and `make srcdist`.
* The Address[] constructor is not used by user apps.
Otherwise we test a test helper method.
|
Thank you. This modifies an existing interface in a patch release ( What do you think @spatula75? |
|
I'm of several minds about it. On the one hand, I hate to annoy people with API-breaking changes, but on the other hand my intuition is that there probably aren't all that many people utilizing this particular API (though I can't prove that... it might be worthwhile for me to post a question on the forum). On the third hand, fixing the breaking API change in this case is trivial... the simple case is just to add an empty method to an existing class. The next best option of creating a second interface for listening to recovery starts would prevent a breaking API change; however, then you have two interfaces that are nearly identical doing almost exactly the same thing, just at a different point in time, but you still have to handle their bookkeeping separately... you end up with some cruft in the code... moreover, if someone is using the existing API, and they wanted to start using the new API, they'd probably just implement both interfaces in one class. And then you end up having to maintain that code forever. It might be preferable to break something once to get the code you want, rather than try to maintain compatibility and have a bunch of code you don't want. I don't like the other two options much at all: making it an abstract class instead of an interface and having an empty implementation for the new method is rather hacky and messy; similarly, making a subinterface and doing instanceof checks in the code to see if you can cast it to the new interface, then calling the new method if you can feels wrong... I usually think of 'instanceof' as an indicator that I'm doing the wrong thing. |
|
@michaelklishin I generally don't like to see such changes, but this should have no impact on Spring AMQP users because it doesn't use the built-in recovery mechanisms, since it's had its own recovery since day one; predating the built-in mechanism. Thanks for considering us, though. It will be nice when, one day, we can set Java 8 as a minimum since you can declare default implementations on interfaces there. |
|
@spatula75 please rebase this against |
|
Sure, no problem. I assume in this case you prefer the first option -- go ahead and do the breaking API change, but against master so that it won't land until the next major release? |
|
(I need time to do proper testing and whatnot anyway.) |
|
@spatula75 correct. Note that master is nearly identical to stable at the moment, and we currently don't have any significant implementation/API changes in mind, so using a build from master is actually much less risky than it sounds. |
…rce support in JDK7 (automatic on compile with JDK7)
|
@spatula75 let me know when you feel comfortable with this change and this PR is rebased to master. We're happy to pull it in for |
|
Just a ping that I didn't forget... work is just a little busy this week into next week, so it may take me a little time to clean up this PR, make sure it's properly tested, and rebased against master. |
|
@spatula75 thank you! |
michaelklishin
changed the title
This provides a simple way to get at the other parameters from the Consumer's `handleDelivery` method (e.g. consumerTag, envelope, properties). It is backwards compatible, and only those using the new interface `responseCall` will have access to the additional data.
|
@spatula75 hi, any updates on this? |
Enhance RpcClient: Provide access to message metadata
…n app wants to log something / set a flag / fire an event when recovery has begun but before it has completed.
|
Managed to find the right incantation / voodoo / animal sacrifice to get Git to perform the desired magic to be branched off Master. I haven't done much of anything in the way of testing to this yet though. |
|
Initial testing looks good; my listener gets called when recovery starts. I don't have the right environment set up to run the full test suite though to make sure I didn't break anything. Will see if I can get that going today. |
|
I wasn't able to get my environment into a state where I could run the automated tests, but I think things should be reasonably close to what they need to be. |
|
@spatula75 thank you. I'd be happy to QA this, can you please rebase this PR so that it cleanly merges into |
|
Hm, looks like this PR contains way more than what the title says. Closing as this should be at least 2 (maybe even 3) separate PRs. |
|
I might have screwed up the rebase if it looks like there's a lot more Would you like me to resubmit a fresh PR, or given our discussion about the On 13 April 2016 at 02:14, Michael Klishin notifications@github.com wrote:
"Courage isn't just a matter of not being frightened, you know. It's being |
|
@spatula75 now that #143 is merged, can you please submit it against master? |
...in case an app wants to log something / set a flag / fire an event when a recovery has begun but before it has completed.
This particular implementation is just an idea / work in progress and hasn't been tested extensively, but I thought it easier to communicate the idea via code than a thousand words trying to describe it.
The major downfall to this approach is that it's a breaking API change to RecoveryListener, though I suspect application use of RecoveryListener is somewhat less-than-common. Another approach might be to make a subinterface of RecoveryListener that holds the extra method (though that's ugly and requires a bunch of instanceof checking when notifying), or changing RecoveryListener to be an abstract class with an empty handleRecoveryStarted method, though this approach isn't exactly pretty either.
Yet another approach would be to create a completely independent interface called RecoveryStartedListener and have them added separately, so they'd be managed independently. This would avoid the breaking API change, but it would add more bookkeeping to the code.
Our use case is that we'd both like to log when a recovery has started and set a flag that a recovery is in progress, then flip the bit back when recovery has completed (and continue to log it as we do now) so that our background task that tries very hard to ensure that consumers keep consuming over dodgy Internet connections can make more intelligent decisions about when to abort & replace a connection (sometimes necessary if an IP address changes, etc., but we'd rather give automatic recovery a chance to work first if it's in progress and likely to succeed).