Multi-threaded topology recovery

[vikinghawk]

Proposed Changes

Decrease recovery time for connections that have lots of queues and bindings by recovering entities (exchanges, queues, bindings, and consumers) in parallel using multiple threads. One of our applications has 100s of channels and 1000s of queues and bindings on a single connection. This PR makes it configurable and opt-in to use multiple threads. Default is still single threaded on the main connection thread. I can add javadoc and clean it up a bit if the team is interested in pulling this in.

In my testing with our workflow I have found recovery time to improve by about 5x using 8 threads vs 1. More than 8 threads seems to have diminishing returns (could be related to the VM i was testing from which only had 2 cores). In one case recovery time went from 26 seconds to 5 seconds and from 119 seconds to 25 in another.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

• Bugfix (non-breaking change which fixes issue #NNNN)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation (correction or otherwise)
• Cosmetics (whitespace, appearance)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in related repositories

Further Comments

[michaelklishin]

This is pretty interesting and I like the way you avoid natural race conditions here. It would be interesting to see what kind of tricky scenarios we can add to the test suite before we put it through a few 100s runs ;)

[michaelklishin]

@vikinghawk @acogoluegnes and I discussed this and we'd like to add a couple of things:

• Make sure ConnectionFactory's thread factory is used when the client has to instantiate an executor service
• Add an option to use a user-provided executor

Also, can you share any background on this or numbers around how much quicker does parallel recovery finish? Thank you!

[acogoluegnes]

Creating a new pool of threads can be a concern in some environments, so maybe we should just add (yet) another executor service in the connection factory. Parallel topology recovery would occur only if this executor service is not null (so we could get rid of the recoveryThreads property).

[vikinghawk]

done

[acogoluegnes]

Maybe we could get the Futures back, check there's the same number as the inputs and they are all done, log a warning if not.

[acogoluegnes]

Check it returns an empty list and log a warning if not?

[vikinghawk]

no longer calling shutdown since the executor is passed in and needs to be reused on subsequent recoveries

[acogoluegnes]

@vikinghawk Thanks for this contribution. I added few comments that worth addressing I guess. The CI build also failed:

Tests run: 403, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1,243.408 sec <<< FAILURE! - in com.rabbitmq.client.test.server.HATests
recoveryWithMultipleThreads(com.rabbitmq.client.test.functional.ConnectionRecovery)  Time elapsed: 11.958 sec  <<< ERROR!
java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
	at com.rabbitmq.client.test.functional.ConnectionRecovery.recoveryWithMultipleThreads(ConnectionRecovery.java:849)

I haven't run it yet, we're kind of busy this week with a team meeting.

[vikinghawk]

changes made as requested. I believe I fixed the test as well, but I haven't had time to setup my new machine yet to run tests.

My testing scenario:
2 node cluster with roughly 600 connections, 4.5k channels,10.5k queues & bindings, 50k consumers, and 300 msgs/second traffic.

Testing on a single connection with 190 channels, 1500 non-durable/auto-delete queues, and 200 durable/mirrored queues. 1 topic exchange binding and 1 consumer per queue.

Threads	Total Recovery Time	Channel Time	Queue Time	Binding Time	Consumer Time
1	26 secs	1	12	11	2
8	5 secs	1	2	2	.5

Testing on a single connection with 190 channels and 1700 durable/mirrored queues. 1 topic exchange binding and 1 consumer per queue.

Note: this client was on a small VM with 2 vCPUs so its possible >8 threads could be beneficial on a bigger machine.

Threads	Total Recovery Time	Channel Time	Queue Time	Binding Time	Consumer Time
1	119 secs	12	82	23	2
4	41 secs	12	22	6	1
8	26 secs	11	11	4	1
16	23 secs	12	6	4	1

I did try to recover Channels in parallel as well at one time, but it did not appear to be any faster so I backed it out. I assume there is some synchronization point on the connection either client or server side.

[michaelklishin]

Thanks, those numbers suggest the difference is pretty significant (which is expected but always nice to see some benchmark evidence). We will continue with QA.

[michaelklishin]

@vikinghawk thank you for the (as always) quality contribution!

[acogoluegnes]

@vikinghawk Merged, thanks! This will be in 4.7.0, we'll produce a release candidate in the next few days.