qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Kennedy <andrewinternatio...@gmail.com>
Subject Re: Request to include QPID-3143 in the 0.10 release
Date Wed, 16 Mar 2011 00:49:17 GMT
On 14 Mar 2011, at 21:41, Rajith Attapattu wrote:
> On Mon, Mar 14, 2011 at 5:33 PM, Andrew Kennedy  
> <andrewinternational@gmail.com> wrote:
>> Ok,
>>
>> From line 1216 of the latest AMQSession.java we have:
>>
>>> if (queueName.indexOf('/') == -1 && queueName.indexOf(';') == -1)
>>> {
>>>    DestSyntax syntax = AMQDestination.getDestType(queueName);
>>>    if (syntax == AMQDestination.DestSyntax.BURL)
>>>    {
>>>        // For testing we may want to use the prefix
>>>        return new AMQQueue(getDefaultQueueExchangeName(),
>>>                new AMQShortString(AMQDestination.stripSyntaxPrefix 
>>> (queueName)));
>>>        }
>>>        else
>>>        {
>>>            AMQQueue queue = new AMQQueue(queueName);
>>>            return queue;
>>>        }
>>>    }
>>>    else
>>>    {
>>>        return new AMQQueue(queueName);
>>>    }
>>> }

Rajith,

I've had time to catch up on the complete session.createQueue  
discussion, and noticed you aren't pushing this for 0.10 inclusion  
anymore. I agree with Rob et al and yourself that we aren't testing  
things correctly.  As I understood things, the (current) incorrect  
behaviour was added here:

	Revision 964984
	http://svn.apache.org/viewvc/qpid/trunk/qpid/java/client/src/main/ 
java/org/apache/qpid/client/AMQSession.java? 
annotate=964984&diff_format=h&pathrev=1081460
	Modified Fri Jul 16 23:50:13 2010 UTC (7 months, 4 weeks ago) by rajith

And was therefore present in 0.8, which I had not realised before. I  
think making the change is the right thing to do, but only once we  
have updated the default test profile and the tests accordingly. If  
the behavioural change had been introduced *post* 0.8, I think there  
would be a stronger case for inclusion.

We probably need to create profiles like this "{broker}-{addressing  
format}-{amqp version}":

	vm-addr-0.10 (default), vm-burl-0.10, java-addr-0.10, java- 
burl-0.10, cpp-addr-0.10, etc.

Or, we could change the test profile mechanism, so you can specify  
something like "-Dprofiles=java,burl,0.10,mina", with (sensible)  
combinations of sub-profiles being valid for the system tests. The  
default set should still be something like "vm,addr,0.10" though.

This is (fairly) easy with Maven ;)

>> [...] should we really not just change this to simply "return new  
>> AMQQueue(queueName);" and then leave everything to the addressing  
>> syntax logic in AMQBindingURL.java and friends instead? It looks  
>> like it should just work, but I haven't tested it. Oh, right, we  
>> get the defaultQueueExchangeName from the *connection* URL, I see.  
>> Well, that's probably more difficult...
>
> The reason is that the old implementation uses the amq.direct as  
> the exchange name (which is what returned by  
> defaultQueueExchangeName.)
> The addressing syntax implementation just uses the default exchange  
> (or the nameless exchange) when dealing with queues.

So, with BURL syntax, you can override the default exchange name on  
the connection URL. But, using ADDR syntax for the destination, there  
is no way of overriding the default exchange name, then, even if we  
change it on the connection URL? Is this right? Or, would you be also  
using an ADDR type URL for the connection in that case?

> Just wanted to make it obvious in the code. Perhaps I should add a  
> comment there as well ?

Yes, good idea.

Andrew.
-- 
-- andrew d kennedy ? do not fold, bend, spindle, or mutilate ;
-- http://grkvlt.blogspot.com/ ? edinburgh : +44 7582 293 255 ;

---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project:      http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org


Mime
View raw message