[rabbitmq-discuss] FW: Multiple consumers

[Ben Hood]

>>>>I am not convinced parameterised modules are the way to go.
>>>>You can tell that something isn't quite right by the large
>>number of
>>>>places amqp_client has different branches for the network
>>and direct
>>>>case.
>>
Do you feel that parameterised modules are not appropriate
generally, or in this specific case?

Would you stance change if the number of different branches
were reduced, e.g. by refactoring the underlying message
passing to make it more consistent between both cases?

The rational behind the parameterization is to reduce as much
AMQP specific message passing down to a lowest common
denominator as possible, so whether than can be achieved by
another means, that would be fine by me.

Just thinking on my feet here, maybe you extract the API
calls to a separate module and then implement separate
gen_server instances for the network case and the direct
case. These separate servers would then implement their own
init/1 and handle_call(open_channel,_) methods. The amqp_send
and amqp_receive functions could be extracted out to amqp_util.

>>>>
>>>>Related to this, your current startup/shutdown sequence is
>>>>
>>>>    AMQPClient = amqp_client:new(amqp_direct_driver),
>>>>    {ok, Connection} = AMQPClient:start(User, Password),
>>>>    Channel = AMQPClient:open_channel(Connection,
>>ChannelNumber, ""),
>>>>    Ticket = AMQPClient:access_request(Channel, Realm),
>>>>    ...
>>>>    AMQPClient:stop(Channel),
>>>>    AMQPClient:stop(Connection).
>>>>
>>>>when I think it should be
>>>>
>>>>    {ok, Connection} = amqp_network_client:start(User, Password),
>>>>    Channel = amqp_connection:open_channel(Connection,
>>>>ChannelNumber, ""),
>>>>    Ticket = amqp_channel:access_request(Channel, Realm)
>>>>    ...
>>>>    amqp_channel:close(Channel),
>>>>    amqp_connection:close(Connection).
>>

I think that would work without too much modification, if
that's what you want. What I am trying to say is that this is
quite doable, I would just like some confirmation of the direction.

>>
>>>>
>>>>The module parameterisation and general client module structure is
>>>>driven largely by the need to support both the direct and network
>>>>client. I reckon we are trying to bite off too much in one go here,
>>>>and suggest concentrating on the network client first.
>>

Fair point. However, it is the direct case that I am
primarily interested in for my own purposes, so with all due
respect, from my personal perspective I would be slightly
reticent to let go of this :-)

Furthermore, I think this is implementable, I just think IMHO
we may be hanging up on the how the client interface is going
to look, which is a soluble problem.

As a suggestion:

{ok, Connection} = amqp_direct_client:start(User, Password),

%% This is where you need to differentiate between the direct and
networked case
Channel = amqp_connection:open_channel(Connection, ChannelNumber, ""),
Ticket = amqp_channel:access_request(Channel, Realm)
    ...
amqp_channel:close(Channel),
amqp_connection:close(Connection).

Where Connection is a {Pid, direct} tuple or a {Pid, network}
tuple as opposed to just a Pid term.

>>>>
>>>>2)
>>>>I wonder whether it would make sense to get rid of most of
>>the channel
>>>>functions and instead get the user to construct requests directly.
>>>>E.g. instead of
>>>>
>>>>    #'queue.declare_ok'{queue = Q1,
>>>>                        message_count = MessageCount,
>>>>                        consumer_count = ConsumerCount}
>>>>                        = AMQPClient:queue_declare(Channel,
>>Ticket, Q)
>>>>
>>>>you'd write
>>>>
>>>>    #'queue.declare_ok'{queue = Q1,
>>>>                        message_count = MessageCount,
>>>>                        consumer_count = ConsumerCount}
>>>>                        = amqp_channel:rpc(
>>>>                            #'queue.declare'{ticket = Ticket,
>>>>                                             queue = binary(Q)}),
>>>>
>>>>This is more symmetric and takes advantage of Erlang's light-weight
>>>>record manipulation syntax. It does however require reasonable
>>>>defaults for all the record fields, which isn't the case
>>right now. As
>>>>an alternative you could define a bunch of record constants
>>that have
>>>>their fields set to reasonable default values and then use record
>>>>update to construct the record instances you need.
>>
>>
I was thinking about this myself because passing in single
parameters and expecting a structured tuple as a return type
is inconsistent, so I was already tending towards the latter.

Again, this is quite reasonable and doable, and I think that
putting the defaults into the record definitions would make
the code a lot cleaner, because clients would only pattern
match on the fields that they know and care about, so it
would also be a lot more *erlangy*, IMHO.

>>>>
>>>>3)
>>>>Why does amqp_client use an ets table instead of a record to record
>>>>the username, password etc?

This was one of the issues I raised against the patch in a
previous post. One of the todo items is to move this over to
a record, I just started with the ets table and left it that
way for now, so this is low hanging fruit.

>>>>
>>>>4)
>>>>The method reading in rabbit_channel should be refactored
>>so that you
>>>>can call it from amqp_receive, rather than duplicating code.
>>>>
>>>>rabbit_reader should be refactored so that
>>amqp_network_driver doesn't
>>>>have to duplicate so much code.
>>
Granted. However, this is IMHO a pure refactoring and
maintenance exercise as opposed to doing something
fundamental, such as the module parameterization discussed above.

Throughout the development of this patch I've tried to find a
balance of code reuse and actually getting a working end to
end for both the network and direct clients. So the emphasis
has been on getting something working in a way that you can
refine what you have within the safety net of unit tests that
make sure that the functionality doesn't break during refactoring.

So unless this is imperative for the acceptance of this patch
into the rabbit tree (which hasn't been discussed yet), I
would take this on as a todo, because I think this would have
to be discussed in context of some other refactoring that
would also be necessary (e.g. moving the portion of  rabbit_*
modules that the client uses into common non-rabbit-specific
modules for redistribution).

>>>>
>>>>
>>>>I am pleased by how little code there is and how little you had to
>>>>change the server modules to support the direct case.
>>>>That will help when carrying this code base forward when we tackle
>>>>AMQP 0-10.
>>>>

This is why I introduced the driver concept, so that I could
use a common message passing sequence specific to the high
level protocol rather than the transport. Maybe calling them
drivers was wrong, perhaps transport is a better term.

Anyway, at some stage I would like to touch base with you to
discuss some issues that are inefficient to discuss over the
list (e.g. patch management).

Ben

*****************************************************
This email is issued by a VocaLink group company. It is confidential
and intended for the exclusive use of the addressee only. You should
not disclose its contents to any other person. If you are not the
addressee (or responsible for delivery of the message to the
addressee), please notify the originator immediately by return message
and destroy the original message. The contents of this email will have
no contractual effect unless it is otherwise agreed between a specific
VocaLink group company and the recipient.

The VocaLink group companies include, among others: VocaLink Limited
(Company No 06119048, [VAT applied for]) which is registered in
England and Wales at registered office Drake House, Homestead Road,
Rickmansworth, WD3 1FX. United Kingdom, Voca Limited (Company no
1023742, VAT No. 226 6112 87) which is registered in England and Wales
at registered office Drake House, Three Rivers Court, Homestead Road,
Rickmansworth, Hertfordshire. WD3 1FX. United Kingdom, LINK
Interchange Network Limited (Company No 3565766, VAT No. 721 6162 62)
which is registered in England and Wales at registered office Arundel
House, 1 Liverpool Gardens, Worthing, West Sussex, BN11 1SL and
VocaLink Holdings Limited (Company No 06119036, [VAT applied for])
which is registered in England and Wales at registered office Drake
House, Homestead Road, Rickmansworth, WD3 1FX. United Kingdom.

The views and opinions expressed in this email may not reflect those
of any member of the VocaLink group. This message and any attachments
have been scanned for viruses prior to leaving the VocaLink group
network; however, VocaLink does not guarantee the security of this
message and will not be responsible for any damages arising as a
result of any virus being passed on or arising from any alteration of
this message by a third party. The VocaLink group may monitor emails
sent to and from the VocaLink group network.

This message has been checked for all email viruses by MessageLabs.
*************************************************************