activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rob Davies <rajdav...@gmail.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 07:22:00 GMT
Hi David,
On 19 Feb 2008, at 05:09, David Sitsky wrote:

> 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)
Yes it should!- changing ...
>
>
> 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?
Don't think so? We only want the message going to  one subscription? I  
may have misunderstood what you mean!

cheers,

Rob
>
>
> -- 
> 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