activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Sitsky <s...@nuix.com>
Subject Re: svn commit: r628667 - in /activemq/trunk/activemq-core/src/main/java/org/apache/activemq/broker/region: ./ policy/
Date Tue, 19 Feb 2008 05:09:20 GMT
Hi Rob,

Great to see these changes.  I am a little confused about this change in 
Queue.java:

> +        if (list != null) {
> +            synchronized (consumers) {      
> +                for (MessageReference node : list) {
> +                    Subscription target = null;
> +                    List<Subscription> targets = null;
> +                    for (Subscription s : consumers) {
> +                        if (dispatchSelector.canSelect(s, node)) {
> +                            if (!s.isFull()) {
> +                                s.add(node);
> +                                target = s;
> +                                break;
> +                            } else {
> +                                if (targets == null) {
> +                                    targets = new ArrayList<Subscription>();
> +                                }
> +                                targets.add(s);
> +                            }
> +                        }
> +                    }
> +                    if (targets != null) {

Shouldn't this if statement be?

if (target == null && targets != null)

I would have thought if you already found a matching subscriber that 
isn't full, then there is no need to check the list of other full 
subscribers that were found?

> +                        // pick the least loaded to add the messag too
> +    
> +                        for (Subscription s : targets) {
> +                            if (target == null
> +                                    || target.getInFlightUsage() > s
> +                                            .getInFlightUsage()) {
> +                                target = s;
> +                            }
> +                        }
> +                        if (target != null) {
> +                            target.add(node);
> +                        }
> +                    }

If what I said above is true, then the immediately above if statement 
needs to be moved outside its enclosing if - otherwise it only gets 
executed when targets != null.  We'd want this to execute if we found a 
matching target wouldn't we?

-- 
Cheers,
David

Nuix Pty Ltd
Suite 79, 89 Jones St, Ultimo NSW 2007, Australia    Ph: +61 2 9280 0699
Web: http://www.nuix.com                            Fax: +61 2 9212 6902

Mime
View raw message