More ActiveMQ hotspots.. courtesy of continuous profiling :)

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

More ActiveMQ hotspots.. courtesy of continuous profiling :)

Kevin Burton
We deployed a continuous profiler at work today and so far the results look
really interesting.

Definitely worth the investment to setup!

Look like we’re spending 50% of our time here:

org.apache.activemq.broker.jmx.ManagedRegionBroker

    @Override
    public void removeConsumer(ConnectionContext context, ConsumerInfo
info) throws Exception {
        for (Subscription sub : subscriptionMap.keySet()) {
            if (sub.getConsumerInfo().equals(info)) {
               // unregister all consumer subs
               unregisterSubscription(subscriptionMap.get(sub), true);
            }
        }
        super.removeConsumer(context, info);
    }

… so I think a patch for that could be to keep an index around for a map
from ConsumerInfo to Subscription… then unregisterSubscription just has to
factor that in as well.

Another hotspot seems to be:

org.apache.activemq.broker.region.Queue.addToConsumerList(Subscription)

We seem to be spending 5% of our CPU time here due to sorting a list, which
I think can be eliminated.

But 5% isn’t that bad.

There’s also this:

org.apache.activemq.broker.region.AbstractRegion.addSubscriptionsForDestination(ConnectionContext,
Destination)

but it was a small impact and looks harder to optimize.


--

Founder/CEO Spinn3r.com
Location: *San Francisco, CA*
blog: http://burtonator.wordpress.com
… or check out my Google+ profile
<https://plus.google.com/102718274791889610666/posts>
Reply | Threaded
Open this post in threaded view
|

Re: More ActiveMQ hotspots.. courtesy of continuous profiling :)

Tim Bain
Kevin,

Great finds.  What tool were you using?

Is it safe to assume you'll submit patches (when you have time) so we don't
need to capture these in JIRA?

Adding a synchronized Multimap<Consumer, Subscription> to
ManagedRegionBroker looked good to me.  It won't be threadsafe against
simultaneous registers/unregisters for the same subscription, but it looks
like that class already has that problem so I don't think that's a reason
not to do it.

In addConsumerToList(), I think we can do a sorted insertion (iterate
through the list till you find the right place based on the comparator,
then insert) to skip the re-sort.  Either we'll be rolling the list or we
won't, but either way the list will be in sorted order, except that the
minimum element might not be the first one.  So find the minimum element's
index (O(log N), but can be optimized to O(1) for the not-rolling case),
then do a sorted insert starting at that index and wrapping if necessary.
On Jun 1, 2015 9:04 PM, "Kevin Burton" <[hidden email]> wrote:

> We deployed a continuous profiler at work today and so far the results look
> really interesting.
>
> Definitely worth the investment to setup!
>
> Look like we’re spending 50% of our time here:
>
> org.apache.activemq.broker.jmx.ManagedRegionBroker
>
>     @Override
>     public void removeConsumer(ConnectionContext context, ConsumerInfo
> info) throws Exception {
>         for (Subscription sub : subscriptionMap.keySet()) {
>             if (sub.getConsumerInfo().equals(info)) {
>                // unregister all consumer subs
>                unregisterSubscription(subscriptionMap.get(sub), true);
>             }
>         }
>         super.removeConsumer(context, info);
>     }
>
> … so I think a patch for that could be to keep an index around for a map
> from ConsumerInfo to Subscription… then unregisterSubscription just has to
> factor that in as well.
>
> Another hotspot seems to be:
>
> org.apache.activemq.broker.region.Queue.addToConsumerList(Subscription)
>
> We seem to be spending 5% of our CPU time here due to sorting a list, which
> I think can be eliminated.
>
> But 5% isn’t that bad.
>
> There’s also this:
>
>
> org.apache.activemq.broker.region.AbstractRegion.addSubscriptionsForDestination(ConnectionContext,
> Destination)
>
> but it was a small impact and looks harder to optimize.
>
>
> --
>
> Founder/CEO Spinn3r.com
> Location: *San Francisco, CA*
> blog: http://burtonator.wordpress.com
> … or check out my Google+ profile
> <https://plus.google.com/102718274791889610666/posts>
>
Reply | Threaded
Open this post in threaded view
|

Re: More ActiveMQ hotspots.. courtesy of continuous profiling :)

Kevin Burton
On Tue, Jun 2, 2015 at 6:16 AM, Tim Bain <[hidden email]> wrote:

> Kevin,
>
> Great finds.  What tool were you using?
>
>
Java Mission Control.. Free for development.. but I think the community
needs a real open source tool.  Pseudo -free isn’t a good idea.


> Is it safe to assume you'll submit patches (when you have time) so we don't
> need to capture these in JIRA?
>

Might want to create one.  The problem with my patch set, is that I’m still
stuck on 5.10.2.  There’s a bug introduced sometime around 5.11 that only
impacts the memory store.  I haven’t been able to track it down yet so I
can’t retarget my patches to head.  That, and I think one of my patches
doesn’t work with LevelDB so I need to fix that.

Realistically, I should fix all those issues so that I can move between
versions.


>
> Adding a synchronized Multimap<Consumer, Subscription> to
> ManagedRegionBroker looked good to me.  It won't be threadsafe against
> simultaneous registers/unregisters for the same subscription, but it looks
> like that class already has that problem so I don't think that's a reason
> not to do it.
>

Or store a second concurrent hash map.  that would fix it.


>
> In addConsumerToList(), I think we can do a sorted insertion (iterate
> through the list till you find the right place based on the comparator,
> then insert) to skip the re-sort.  Either we'll be rolling the list or we
> won't, but either way the list will be in sorted order, except that the
> minimum element might not be the first one.  So find the minimum element's
> index (O(log N), but can be optimized to O(1) for the not-rolling case),
> then do a sorted insert starting at that index and wrapping if necessary.


True.. That would definitely make that faster.

Kevin
--

Founder/CEO Spinn3r.com
Location: *San Francisco, CA*
blog: http://burtonator.wordpress.com
… or check out my Google+ profile
<https://plus.google.com/102718274791889610666/posts>
Reply | Threaded
Open this post in threaded view
|

Re: More ActiveMQ hotspots.. courtesy of continuous profiling :)

Thiago Kronig
YourKit <https://www.yourkit.com/> and JProfiler <
http://www.ej-technologies.com/products/jprofiler/overview.html> are great
commercial tools, and they have licenses for open source projects.

On Tue, Jun 2, 2015 at 11:19 AM Kevin Burton <[hidden email]> wrote:

> On Tue, Jun 2, 2015 at 6:16 AM, Tim Bain <[hidden email]> wrote:
>
> > Kevin,
> >
> > Great finds.  What tool were you using?
> >
> >
> Java Mission Control.. Free for development.. but I think the community
> needs a real open source tool.  Pseudo -free isn’t a good idea.
>
>
> > Is it safe to assume you'll submit patches (when you have time) so we
> don't
> > need to capture these in JIRA?
> >
>
> Might want to create one.  The problem with my patch set, is that I’m still
> stuck on 5.10.2.  There’s a bug introduced sometime around 5.11 that only
> impacts the memory store.  I haven’t been able to track it down yet so I
> can’t retarget my patches to head.  That, and I think one of my patches
> doesn’t work with LevelDB so I need to fix that.
>
> Realistically, I should fix all those issues so that I can move between
> versions.
>
>
> >
> > Adding a synchronized Multimap<Consumer, Subscription> to
> > ManagedRegionBroker looked good to me.  It won't be threadsafe against
> > simultaneous registers/unregisters for the same subscription, but it
> looks
> > like that class already has that problem so I don't think that's a reason
> > not to do it.
> >
>
> Or store a second concurrent hash map.  that would fix it.
>
>
> >
> > In addConsumerToList(), I think we can do a sorted insertion (iterate
> > through the list till you find the right place based on the comparator,
> > then insert) to skip the re-sort.  Either we'll be rolling the list or we
> > won't, but either way the list will be in sorted order, except that the
> > minimum element might not be the first one.  So find the minimum
> element's
> > index (O(log N), but can be optimized to O(1) for the not-rolling case),
> > then do a sorted insert starting at that index and wrapping if necessary.
>
>
> True.. That would definitely make that faster.
>
> Kevin
> --
>
> Founder/CEO Spinn3r.com
> Location: *San Francisco, CA*
> blog: http://burtonator.wordpress.com
> … or check out my Google+ profile
> <https://plus.google.com/102718274791889610666/posts>
>
Reply | Threaded
Open this post in threaded view
|

Re: More ActiveMQ hotspots.. courtesy of continuous profiling :)

Kevin Burton
Both are decent... Just not really for production use. One of the cool
things about java mission control is the overhead is only about one
percent. I would love to see other profilers catch  up.
On Jun 2, 2015 7:26 AM, "Thiago Kronig" <[hidden email]> wrote:

> YourKit <https://www.yourkit.com/> and JProfiler <
> http://www.ej-technologies.com/products/jprofiler/overview.html> are great
> commercial tools, and they have licenses for open source projects.
>
> On Tue, Jun 2, 2015 at 11:19 AM Kevin Burton <[hidden email]> wrote:
>
> > On Tue, Jun 2, 2015 at 6:16 AM, Tim Bain <[hidden email]> wrote:
> >
> > > Kevin,
> > >
> > > Great finds.  What tool were you using?
> > >
> > >
> > Java Mission Control.. Free for development.. but I think the community
> > needs a real open source tool.  Pseudo -free isn’t a good idea.
> >
> >
> > > Is it safe to assume you'll submit patches (when you have time) so we
> > don't
> > > need to capture these in JIRA?
> > >
> >
> > Might want to create one.  The problem with my patch set, is that I’m
> still
> > stuck on 5.10.2.  There’s a bug introduced sometime around 5.11 that only
> > impacts the memory store.  I haven’t been able to track it down yet so I
> > can’t retarget my patches to head.  That, and I think one of my patches
> > doesn’t work with LevelDB so I need to fix that.
> >
> > Realistically, I should fix all those issues so that I can move between
> > versions.
> >
> >
> > >
> > > Adding a synchronized Multimap<Consumer, Subscription> to
> > > ManagedRegionBroker looked good to me.  It won't be threadsafe against
> > > simultaneous registers/unregisters for the same subscription, but it
> > looks
> > > like that class already has that problem so I don't think that's a
> reason
> > > not to do it.
> > >
> >
> > Or store a second concurrent hash map.  that would fix it.
> >
> >
> > >
> > > In addConsumerToList(), I think we can do a sorted insertion (iterate
> > > through the list till you find the right place based on the comparator,
> > > then insert) to skip the re-sort.  Either we'll be rolling the list or
> we
> > > won't, but either way the list will be in sorted order, except that the
> > > minimum element might not be the first one.  So find the minimum
> > element's
> > > index (O(log N), but can be optimized to O(1) for the not-rolling
> case),
> > > then do a sorted insert starting at that index and wrapping if
> necessary.
> >
> >
> > True.. That would definitely make that faster.
> >
> > Kevin
> > --
> >
> > Founder/CEO Spinn3r.com
> > Location: *San Francisco, CA*
> > blog: http://burtonator.wordpress.com
> > … or check out my Google+ profile
> > <https://plus.google.com/102718274791889610666/posts>
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: More ActiveMQ hotspots.. courtesy of continuous profiling :)

Kevin Burton
Btw.. it looks like you can set useConsumerPriority=false on the
destination policy entry and get a free 5% performance boost.



On Tue, Jun 2, 2015 at 8:16 AM, Kevin Burton <[hidden email]> wrote:

> Both are decent... Just not really for production use. One of the cool
> things about java mission control is the overhead is only about one
> percent. I would love to see other profilers catch  up.
> On Jun 2, 2015 7:26 AM, "Thiago Kronig" <[hidden email]> wrote:
>
>> YourKit <https://www.yourkit.com/> and JProfiler <
>> http://www.ej-technologies.com/products/jprofiler/overview.html> are
>> great
>> commercial tools, and they have licenses for open source projects.
>>
>> On Tue, Jun 2, 2015 at 11:19 AM Kevin Burton <[hidden email]> wrote:
>>
>> > On Tue, Jun 2, 2015 at 6:16 AM, Tim Bain <[hidden email]> wrote:
>> >
>> > > Kevin,
>> > >
>> > > Great finds.  What tool were you using?
>> > >
>> > >
>> > Java Mission Control.. Free for development.. but I think the community
>> > needs a real open source tool.  Pseudo -free isn’t a good idea.
>> >
>> >
>> > > Is it safe to assume you'll submit patches (when you have time) so we
>> > don't
>> > > need to capture these in JIRA?
>> > >
>> >
>> > Might want to create one.  The problem with my patch set, is that I’m
>> still
>> > stuck on 5.10.2.  There’s a bug introduced sometime around 5.11 that
>> only
>> > impacts the memory store.  I haven’t been able to track it down yet so I
>> > can’t retarget my patches to head.  That, and I think one of my patches
>> > doesn’t work with LevelDB so I need to fix that.
>> >
>> > Realistically, I should fix all those issues so that I can move between
>> > versions.
>> >
>> >
>> > >
>> > > Adding a synchronized Multimap<Consumer, Subscription> to
>> > > ManagedRegionBroker looked good to me.  It won't be threadsafe against
>> > > simultaneous registers/unregisters for the same subscription, but it
>> > looks
>> > > like that class already has that problem so I don't think that's a
>> reason
>> > > not to do it.
>> > >
>> >
>> > Or store a second concurrent hash map.  that would fix it.
>> >
>> >
>> > >
>> > > In addConsumerToList(), I think we can do a sorted insertion (iterate
>> > > through the list till you find the right place based on the
>> comparator,
>> > > then insert) to skip the re-sort.  Either we'll be rolling the list
>> or we
>> > > won't, but either way the list will be in sorted order, except that
>> the
>> > > minimum element might not be the first one.  So find the minimum
>> > element's
>> > > index (O(log N), but can be optimized to O(1) for the not-rolling
>> case),
>> > > then do a sorted insert starting at that index and wrapping if
>> necessary.
>> >
>> >
>> > True.. That would definitely make that faster.
>> >
>> > Kevin
>> > --
>> >
>> > Founder/CEO Spinn3r.com
>> > Location: *San Francisco, CA*
>> > blog: http://burtonator.wordpress.com
>> > … or check out my Google+ profile
>> > <https://plus.google.com/102718274791889610666/posts>
>> >
>>
>


--

Founder/CEO Spinn3r.com
Location: *San Francisco, CA*
blog: http://burtonator.wordpress.com
… or check out my Google+ profile
<https://plus.google.com/102718274791889610666/posts>
Reply | Threaded
Open this post in threaded view
|

Re: More ActiveMQ hotspots.. courtesy of continuous profiling :)

Tim Bain
In reply to this post by Kevin Burton
On Tue, Jun 2, 2015 at 8:17 AM, Kevin Burton <[hidden email]> wrote:

> On Tue, Jun 2, 2015 at 6:16 AM, Tim Bain <[hidden email]> wrote:
>
> > Kevin,
> >
> > Great finds.  What tool were you using?
> >
> >
> Java Mission Control.. Free for development.. but I think the community
> needs a real open source tool.  Pseudo -free isn’t a good idea.
>

I'd heard good things but hadn't yet checked it out; guess I should.


> > Is it safe to assume you'll submit patches (when you have time) so we
> don't
> > need to capture these in JIRA?
> >
>
> Might want to create one.


https://issues.apache.org/jira/browse/AMQ-5824
https://issues.apache.org/jira/browse/AMQ-5825


> The problem with my patch set, is that I’m still
> stuck on 5.10.2.  There’s a bug introduced sometime around 5.11 that only
> impacts the memory store.  I haven’t been able to track it down yet so I
> can’t retarget my patches to head.


Can you provide more details?  We're using the memory store in 5.10.0, so
we'd like to know what might bite us if we upgrade.


> That, and I think one of my patches
> doesn’t work with LevelDB so I need to fix that.
>
> Realistically, I should fix all those issues so that I can move between
> versions.
>

Excellent plan.  :)


> > Adding a synchronized Multimap<Consumer, Subscription> to
> > ManagedRegionBroker looked good to me.  It won't be threadsafe against
> > simultaneous registers/unregisters for the same subscription, but it
> looks
> > like that class already has that problem so I don't think that's a reason
> > not to do it.
> >
>
> Or store a second concurrent hash map.  that would fix it.
>

It wouldn't; concurrent collections only protect you from corrupting the
collection itself due to concurrent modifications; they don't protect you
from synchronization issues between multiple collections like you'd have
here (since you'll want to always have an element in neither collection or
in both, but without explicit locking you could have it in one but not the
other) and they don't protect you from synchronization issues where you
observe the current state of the collection and act upon it (where it might
have changed between your observation and your action).

I've generally found that concurrent collections hurt more than they help,
mainly because developers assume they're magically safe in multithreaded
applications when they're not.  Guess maybe I should just switch to Scala
so I don't have to worry about any of that...

> In addConsumerToList(), I think we can do a sorted insertion (iterate
> > through the list till you find the right place based on the comparator,
> > then insert) to skip the re-sort.  Either we'll be rolling the list or we
> > won't, but either way the list will be in sorted order, except that the
> > minimum element might not be the first one.  So find the minimum
> element's
> > index (O(log N), but can be optimized to O(1) for the not-rolling case),
> > then do a sorted insert starting at that index and wrapping if necessary.
>
>
> True.. That would definitely make that faster.
>
> Kevin
> --
>
> Founder/CEO Spinn3r.com
> Location: *San Francisco, CA*
> blog: http://burtonator.wordpress.com
> … or check out my Google+ profile
> <https://plus.google.com/102718274791889610666/posts>
>
Reply | Threaded
Open this post in threaded view
|

Re: More ActiveMQ hotspots.. courtesy of continuous profiling :)

Kevin Burton
Thanks for creating the issues!


> > The problem with my patch set, is that I’m still
> > stuck on 5.10.2.  There’s a bug introduced sometime around 5.11 that only
> > impacts the memory store.  I haven’t been able to track it down yet so I
> > can’t retarget my patches to head.
>
>
> Can you provide more details?  We're using the memory store in 5.10.0, so
> we'd like to know what might bite us if we upgrade.
>
>
Advisories break when using the memory store. A warning that a null pointer
exception was caught goes to the log but the advisories aren’t raised.

I tried to git bisect it but wasn’t able to duplicate it easily. I think
because of some git idiosyncrasy prevented me from finding the commit that
broke.


> It wouldn't; concurrent collections only protect you from corrupting the
> collection itself due to concurrent modifications; they don't protect you
> from synchronization issues between multiple collections like you'd have
> here (since you'll want to always have an element in neither collection or
> in both, but without explicit locking you could have it in one but not the
> other) and they don't protect you from synchronization issues where you
> observe the current state of the collection and act upon it (where it might
> have changed between your observation and your action).
>
>
I had thought about that.  I assumed that the multi map was implemented as
two concurrent maps.  If each operation is concurrent and atomic then I’m
fine with that solution.

I think all we need is that the method() is atomic, not the underlying
data.  Maybe I’m wrong though.  Perhaps there’s a race where we can try to
remove something while it’s being added?


> I've generally found that concurrent collections hurt more than they help,
> mainly because developers assume they're magically safe in multithreaded
> applications when they're not.  Guess maybe I should just switch to Scala
> so I don't have to worry about any of that...
>
>
Perhaps.. but a lot of these things are called out in the documentation.

Kevin

--

Founder/CEO Spinn3r.com
Location: *San Francisco, CA*
blog: http://burtonator.wordpress.com
… or check out my Google+ profile
<https://plus.google.com/102718274791889610666/posts>
Reply | Threaded
Open this post in threaded view
|

Re: More ActiveMQ hotspots.. courtesy of continuous profiling :)

Tim Bain
On Wed, Jun 3, 2015 at 1:16 PM, Kevin Burton <[hidden email]> wrote:

> Thanks for creating the issues!
>

No problem.


> > > The problem with my patch set, is that I’m still
> > > stuck on 5.10.2.  There’s a bug introduced sometime around 5.11 that
> only
> > > impacts the memory store.  I haven’t been able to track it down yet so
> I
> > > can’t retarget my patches to head.
> >
> >
> > Can you provide more details?  We're using the memory store in 5.10.0, so
> > we'd like to know what might bite us if we upgrade.
> >
> >
> Advisories break when using the memory store. A warning that a null pointer
> exception was caught goes to the log but the advisories aren’t raised.
>
> I tried to git bisect it but wasn’t able to duplicate it easily. I think
> because of some git idiosyncrasy prevented me from finding the commit that
> broke.
>

OK, thanks for sharing.  Have you created a bug report for it?  If not, can
you do that so it doesn't get lost?


> > It wouldn't; concurrent collections only protect you from corrupting the
> > collection itself due to concurrent modifications; they don't protect you
> > from synchronization issues between multiple collections like you'd have
> > here (since you'll want to always have an element in neither collection
> or
> > in both, but without explicit locking you could have it in one but not
> the
> > other) and they don't protect you from synchronization issues where you
> > observe the current state of the collection and act upon it (where it
> might
> > have changed between your observation and your action).
> >
> >
> I had thought about that.  I assumed that the multi map was implemented as
> two concurrent maps.  If each operation is concurrent and atomic then I’m
> fine with that solution.
>

A Guava Multimap is essentially a Map<Key, List<Value>>, but without having
to manage the Lists yourself.  It lets you map each key to an unlimited
number of values, while preserving the simple semantics of the fundamental
Map operations.  It's not a pair of maps (which is what it sounds like you
were expecting; if I misunderstood, I apologize) nor is it essentially a
Map<Key, Pair<ValueType1, ValueType2>>; it holds one homogenous set of
values that just happen to have overlapping keys, not multiple sets of
values where each key pairs with one value per set.

You could probably implement something that held both value types for a
given key with a Multimap, but you'd have to use Object as your value class
to accommodate the multiple value types and you'd have to test each one's
type to figure out what role it's supposed to be used in, so I don't think
you'd want to.  And although for a concurrent Multimap it's presumably
possible to atomically remove all values for a key (though I've not needed
to yet so I haven't looked), I don't believe it's possible to add multiple
values for a key atomically, so I'm not sure it's possible even if you were
willing to hold your nose while you did it.


> I think all we need is that the method() is atomic, not the underlying
> data.  Maybe I’m wrong though.  Perhaps there’s a race where we can try to
> remove something while it’s being added?
>

That's what I'm not sure about; I don't know whether there are situations
where we can concurrently attempt to add and remove consumers (maybe in the
scenario where a broker-to-broker network connection fails over?), nor
whether there are situations where we might try to get values from both
maps but where only one of the two is populated because we're in the middle
of an addition or a deletion.  Maybe someone else who's got more experience
than me with that code can say whether it's a concern...


> > I've generally found that concurrent collections hurt more than they
> help,
> > mainly because developers assume they're magically safe in multithreaded
> > applications when they're not.  Guess maybe I should just switch to Scala
> > so I don't have to worry about any of that...
> >
> >
> Perhaps.. but a lot of these things are called out in the documentation.
>
> Kevin
>

If only every developer actually read the documentation...

Tim
Reply | Threaded
Open this post in threaded view
|

Re: More ActiveMQ hotspots.. courtesy of continuous profiling :)

Kevin Burton
> Advisories break when using the memory store. A warning that a null
pointer

> > exception was caught goes to the log but the advisories aren’t raised.
>
>
> OK, thanks for sharing.  Have you created a bug report for it?  If not, can
> you do that so it doesn't get lost?
>
>
I think I did.. I will verify.


>
> That's what I'm not sure about; I don't know whether there are situations
> where we can concurrently attempt to add and remove consumers (maybe in the
> scenario where a broker-to-broker network connection fails over?), nor
> whether there are situations where we might try to get values from both
> maps but where only one of the two is populated because we're in the middle
> of an addition or a deletion.  Maybe someone else who's got more experience
> than me with that code can say whether it's a concern..
>

Another thing I was thinking about was the ability to just disable this
feature in JMX.  But maybe that’s not needed if it’s eventually fixed.

--

Founder/CEO Spinn3r.com
Location: *San Francisco, CA*
blog: http://burtonator.wordpress.com
… or check out my Google+ profile
<https://plus.google.com/102718274791889610666/posts>
Reply | Threaded
Open this post in threaded view
|

Re: More ActiveMQ hotspots.. courtesy of continuous profiling :)

Tim Bain
On Mon, Jun 8, 2015 at 9:43 AM, Kevin Burton <[hidden email]> wrote:

> > Advisories break when using the memory store. A warning that a null
> pointer
>
> > > exception was caught goes to the log but the advisories aren’t raised.
> >
> >
> > OK, thanks for sharing.  Have you created a bug report for it?  If not,
> can
> > you do that so it doesn't get lost?
> >
> >
> I think I did.. I will verify.
>
>
> >
> > That's what I'm not sure about; I don't know whether there are situations
> > where we can concurrently attempt to add and remove consumers (maybe in
> the
> > scenario where a broker-to-broker network connection fails over?), nor
> > whether there are situations where we might try to get values from both
> > maps but where only one of the two is populated because we're in the
> middle
> > of an addition or a deletion.  Maybe someone else who's got more
> experience
> > than me with that code can say whether it's a concern..
> >
>
> Another thing I was thinking about was the ability to just disable this
> feature in JMX.  But maybe that’s not needed if it’s eventually fixed.
>

Allowing it to be turned on and off feels like overkill in this case, so
I'd say just implement the fix without the option to turn it on/off.  No
one would know when it was appropriate to use it (or not), so don't give
people the option.

Tim