[rabbitmq-discuss] FW: Multiple consumers

[Matthias Radestock]

Ben,

"Ben Hood" <0x6e6562 at gmail.com> writes:

> Here's the patch for the 1.1.0 release.

Some comments:

1)
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.

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).

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.

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.

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

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.


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.


Matthias