I'm not sure that moving the MapHandler inteface from broker to amqp is correct.

The interface is used by the protocol independent code to call visitors to the property maps in messages so I'd expect the interface itself to also be a protocol independent thing. The only other type that is affected is CharSequence and this is very simple and so could be in broker too.

This change now has the effect of make the old 0_10 protocol implementation dependent on the new 1.0 implementation which strikes me as a bad idea (and easy to fix at this stage).

Andrew Stitcher
added a comment - 04/Jun/13 17:23 I'm not sure that moving the MapHandler inteface from broker to amqp is correct.
The interface is used by the protocol independent code to call visitors to the property maps in messages so I'd expect the interface itself to also be a protocol independent thing. The only other type that is affected is CharSequence and this is very simple and so could be in broker too.
This change now has the effect of make the old 0_10 protocol implementation dependent on the new 1.0 implementation which strikes me as a bad idea (and easy to fix at this stage).

The qpid::amqp code is not part of 1.0 specific libraries, its compiled in to qpid common. The same interface is useful in both the client and the broker which is why I moved it. What package would you prefer? (I considered qpid::types, but this shouldn't be part of any public API/ABI yet and that package is compile into a separate library).

Gordon Sim
added a comment - 04/Jun/13 17:37 The qpid::amqp code is not part of 1.0 specific libraries, its compiled in to qpid common. The same interface is useful in both the client and the broker which is why I moved it. What package would you prefer? (I considered qpid::types, but this shouldn't be part of any public API/ABI yet and that package is compile into a separate library).

Ah, I assumed that qpid::amqp was specific to the new amqp support code.

My mental model was that you had chosen to call the new amqp 1.0 support just qpid::amqp because it supports the official standardised protocol rather than the pre-standard 0.10 etc. versions - this makes reasonable sense to me.

But if I look at the code in qpid::amqp much of it is actually 1.0 specific irrespective of which library it gets built into.

At present the only place in the current tree which isn't protocol specific and is shared between broker and client is qpid::sys which isn't a good name at all (and there is indeed stuff in there that isn't actually shared!)

It would definitely be good to have a good set of rules about what goes where in the tree and then to move everything to suit.

Andrew Stitcher
added a comment - 04/Jun/13 17:48 Ah, I assumed that qpid::amqp was specific to the new amqp support code.
My mental model was that you had chosen to call the new amqp 1.0 support just qpid::amqp because it supports the official standardised protocol rather than the pre-standard 0.10 etc. versions - this makes reasonable sense to me.
But if I look at the code in qpid::amqp much of it is actually 1.0 specific irrespective of which library it gets built into.
At present the only place in the current tree which isn't protocol specific and is shared between broker and client is qpid::sys which isn't a good name at all (and there is indeed stuff in there that isn't actually shared!)
It would definitely be good to have a good set of rules about what goes where in the tree and then to move everything to suit.

I agree that there is nothing that ties MapHandler (or CharSequence) to amqp specifically. It's there now mainly because I couldn't see a better place at present (I also agree the existing structure in general is not ideal). Possibly they could form the start of a new namespace - not sure what that should be called though...

Gordon Sim
added a comment - 04/Jun/13 18:33 I agree that there is nothing that ties MapHandler (or CharSequence) to amqp specifically. It's there now mainly because I couldn't see a better place at present (I also agree the existing structure in general is not ideal). Possibly they could form the start of a new namespace - not sure what that should be called though...