| Author |
Message |
|
| 0x6e6562 |
 Posted: Wed Aug 01, 2007 6:32 pm |
 |
|
|
User
Joined: 12 Jul 2007
Posts: 250
|
Matthias,
> Here is the latest patch based on the design discussion we have been
> having. This demonstrates how the connection initiation, opening
Sorry, I didn't realize that you were going to release 1.1.0 so soon.
I'll download that and incorporate the client into that and resubmit
the patch.
Ben
_______________________________________________
rabbitmq-discuss mailing list
rabbitmq-discuss@lists.rabbitmq.com
http://lists.rabbitmq.com/cgi-bin/mailman/listinfo/rabbitmq-discuss
Post recived from mailinglist |
|
|
| Back to top |
 
|
| 0x6e6562 |
| Posted: Wed Aug 01, 2007 8:08 pm |
|
|
|
User
Joined: 12 Jul 2007
Posts: 250
|
Sorry, I forwarded this accidentally to erlang-questions....
---------- Forwarded message ----------
From: Ben Hood <0x6e6562@gmail.com>
Date: Aug 1, 2007 12:53 AM
Subject: Re: [rabbitmq-discuss] FW: Multiple consumers
To: Matthias Radestock <matthias@lshift.net>
Matthias,
Here is the latest patch based on the design discussion we have been
having. This demonstrates how the connection initiation, opening
channel 0, opening channel 1 and doing an access request for both the
direct and networked cases. Please bear in mind that I was
concentrating on getting a working prototype to you based on the
design you described, so there are a lot of rough edges that require
refactoring, cleaning up and polishing, as well as going through the
complete spec for each case (direct and network).
You can best see the flow by executing the amqp_client_test module
which has a test case for the direct and network cases.
To get a unified API, what I have done is to parameterize the client
with a direct driver and a network driver that implement the necessary
callbacks to provide connectivity in the 2 scenarios
(amqp_direct_driver and amqp_network_driver). By doing this I can use
amqp_send() and amqp_receive() functions in the higher level client in
a transparent fashion, so the actual protocol handling is common.
In the direct case, a new process for a client channel is spawned and
this maintains connectivity to the server peer.
In the network case, a new process for the initial AMQP network
connection is spawned which handles the connection tuning. This
process handles the socket connectivity (to be exact, there's a reader
process and a writer process). Then when a user calls open_channel, a
new amqp_client process is spawned for that channel, this process is
registered with the relevant socket reader/writer processes, and
normal message passing occurs from here on in. This way the same
send() and recv() for AMQP can be used in the direct and network
cases.
There are a lot of things that require attention, for example
- The way the new channel processes are send to the socket reader (I
have been reading the gen_tcp man pages for a cleaner way to do this)
- Making sure that exits are trapped correctly
- Taking out all of the io:format() statements (is a logging strategy
worth considering?)
- Streamlining the messages that get passed, so the amqp_send/receive
functions are simpler and easier to understand (there's probably
redundancy in there) and to get rid of some case statements that
currently exist.
- General refactoring in the client to make it neater and easier to maintain.
- Considering refactoring commonalities between the client and the
server, such as the header files and the server modules used by the
client
- I patched rabbit_framing.erl, which I have subsequently learned is a
generated file, so that needs cleaning up.
- The patch probably also contains changes to server modules that are
not entirely necessary, these need to be identified and reviewed.
- Do some real testing with it 
- Benchmark the direct and network drivers to see what difference it
makes, if any 
But I just wanted to find out if this is going in the right direction
before I do too much on these points. Also, there are some
dependencies on refactoring the server modules.
So I'll just answer some of your comments now:
> > 1. In the direct case, the host and channel would be set in the
> > start() call, so just passing the Pid is fine.
>
> There is no 'host' in the direct case.
Fine. This is how it has been implemented.
> The initialisation sequence for the networked client is:
>
> 1) open socket,
>
> 2) spawn reader process, parameterised by the socket.
>
> 3) open channels by sending a message to the reader process. This should
> result in the spawning a new channel process, parameterised by the
> channel number, ReaderPid and WriterPid, and the necessary book-keeping
> (mapping channel numbers to pids) - i.e. everything
> rabbit_reader:open_channel does currently. The reply message to the
> sender of the 'open_channel' message should contain the Pid of the new
> channel process.
This is IMHO exactly how it works now.
>
> > 2. Should the processes be linked at all? That is, if a channel
> > process dies, should the user process die as well?
This requires some review to check that it is done properly in the
code, but is not really a big deal.
>
> Channel processes should be (and are) linked to their reader process. As
> for linking them to the user process, i.e. the process that initiated
> the opening of a channel, I reckon we should leave that up to the user.
OK.
HTH,
Ben
Post recived from mailinglist |
|
|
| Back to top |
|
| Guest |
| Posted: Wed Aug 01, 2007 8:11 pm |
|
|
|
Guest
|
|
| Back to top |
|
| 0x6e6562 |
| Posted: Fri Aug 03, 2007 6:37 pm |
|
|
|
User
Joined: 12 Jul 2007
Posts: 250
|
Matthias,
> > Sorry, I didn't realize that you were going to release 1.1.0 so soon.
> > I'll download that and incorporate the client into that and resubmit
> > the patch.
Here's the patch for the 1.1.0 release. Conceptually it is no
different than the patch I submitted earlier this week, albeit for the
fact that a lot of methods are now implemented for both the direct and
network cases, so my previous observations still hold water.
This patch implements the following API methods (which have been
tested with both the network and direct variants):
open_channel/3
access_request/2
queue_declare/3
exchange_declare/8
queue_bind/5
basic_publish/6
basic_consume/4
basic_get/4
In addition to my previous observations, the way the consumer tag is
mapped to a consumer pid during the delivery in the network case needs
some attention.
HTH,
Ben
Post recived from mailinglist |
|
|
| Back to top |
|
| 0x6e6562 |
| Posted: Sun Aug 05, 2007 7:45 pm |
|
|
|
User
Joined: 12 Jul 2007
Posts: 250
|
> In addition to my previous observations, the way the consumer tag is
> mapped to a consumer pid during the delivery in the network case needs
> some attention.
I've been investigating this and I've noticed a problem with
basic.cancel methods in the direct API. Should this method be sent to
channel, in order that the channel cancels the client or should the
message be sent directly to the consumer? Should the direct channel
maintain a list of consumers, even if it is not necessarily
interacting with them in the normal course of events (because the
server channel dispatches directly to the subscriber)? What are the
advantages/disadvantages?
Ben
_______________________________________________
rabbitmq-discuss mailing list
rabbitmq-discuss@lists.rabbitmq.com
http://lists.rabbitmq.com/cgi-bin/mailman/listinfo/rabbitmq-discuss
Post recived from mailinglist |
|
|
| Back to top |
|
| Guest |
| Posted: Mon Aug 06, 2007 9:08 am |
|
|
|
Guest
|
Ben,
"Ben Hood" <0x6e6562@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
_______________________________________________
rabbitmq-discuss mailing list
rabbitmq-discuss@lists.rabbitmq.com
http://lists.rabbitmq.com/cgi-bin/mailman/listinfo/rabbitmq-discuss
Post recived from mailinglist |
|
|
| Back to top |
|
| 0x6e6562 |
| Posted: Mon Aug 06, 2007 6:19 pm |
|
|
|
User
Joined: 12 Jul 2007
Posts: 250
|
>>>>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.
*************************************************************
_______________________________________________
rabbitmq-discuss mailing list
rabbitmq-discuss@lists.rabbitmq.com
http://lists.rabbitmq.com/cgi-bin/mailman/listinfo/rabbitmq-discuss
Post recived from mailinglist |
|
|
| Back to top |
|
| 0x6e6562 |
| Posted: Tue Aug 07, 2007 4:39 pm |
|
|
|
User
Joined: 12 Jul 2007
Posts: 250
|
> > Why does amqp_client use an ets table instead of a record to record the
> > username, password etc?
>
> I'll look into this as another patch in the stack next as this is low
> hanging fruit.
Here's an incremental patch that solves this. Let me know if you need
a full patch.
HTH,
Ben
Post recived from mailinglist |
|
|
| Back to top |
|
| 0x6e6562 |
| Posted: Tue Aug 07, 2007 4:40 pm |
|
|
|
User
Joined: 12 Jul 2007
Posts: 250
|
Matthias,
I've reworked your comments into this patch, which I've responded to
inline. In essence, the amqp_client module has been thinned and
de-parameterized and I've introduced the modules amqp_connection and
amqp_channel.
BTW, since this is get quite patchy, I've started to use a patch stack
to keep things maintainable and separated, so you can have any patch
as a diff of the vanilla 1.1.0 tree or as a patch series, whichever
you'd prefer.
> 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.
--snip--
> 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.
I've removed the parameterization so the user code looks like this:
{ok, Connection} = amqp_connection:start(User, Password, Host),
Channel = amqp_connection:open_channel(Connection, ChannelNumber, ""),
amqp_channel:access_request(Channel, Realm),
%%...
amqp_channel:stop(Channel),
amqp_connection:stop(Connection).
This differs only marginally from your suggestion in that I don't use
a separate amqp_network_client module, because when I did so, I
realized that this was very thin and in actual fact quite tied to the
amqp_connection module.
Part of the root cause for the parameterization was the direct/network
branching in the amqp_client akin to ifdef macros in C. I've reduced
the use of these and made them dependent on an internal flag, so they
haven't gone away (yet). Whether they should completely disappear is a
matter of design - amqp_send could be implemented in a specific driver
rather than the generic client, but I think that may be 6 of one and
half a dozen of the other. I think this is something we need to
discuss.
> 2)
> 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)}),
I'll look into this as another patch in the stack, with the 2nd
highest priority.
> Why does amqp_client use an ets table instead of a record to record the
> username, password etc?
I'll look into this as another patch in the stack next as this is low
hanging fruit.
Ben
Post recived from mailinglist |
|
|
| Back to top |
|
| 0x6e6562 |
| Posted: Wed Aug 08, 2007 9:38 pm |
|
|
|
User
Joined: 12 Jul 2007
Posts: 250
|
Matthias,
> 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.
This (incremental) patch introduces the concept of an internal rpc
mechanism for the amqp_client and thereby simplifies the generic
message passing to the network and direct drivers as well as making
their differentation virtually transparent (apart from one case that I
think I can refactor out as well). It also thins the amqp_client
module by 30 lines or so.
HTH,
Ben
Post recived from mailinglist |
|
|
| Back to top |
|
|
|
All times are GMT
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
You cannot attach files in this forum
You cannot download files in this forum
|
|
|
Digg It
Del.icio.us
Reddit
Facebook
Stumble Upon
Technorati
