[rabbitmq-discuss] Spying on "mandatory" messages

Fri Mar 25 14:58:43 GMT 2011

Charly,

This is most impressive! I have a few comments on the code though...

On Fri, Mar 25, 2011 at 03:15:03PM +0100, Charly Hamy wrote:
> *rabbit_amqqueue.erl :*
> *(line 115, spec of deliver/2 edited )*
> 
> * -spec(deliver/2 :: (pid(), rabbit_types:delivery()) -> boolean()|spy).*

Traditionally, atoms in specs should be quoted, so 'spy', not spy. This
has no bearing on much other than for consistency with the rest of the
code base (and perhaps syntax highlighting too).

> declare(QueueName, Durable, AutoDelete, Args, Owner) ->
>     ok = check_declare_arguments(QueueName, Args),
>     Q = start_queue_process(#amqqueue{name = QueueName,
>                                       durable = Durable,
>                                       auto_delete = AutoDelete,
>                                       arguments = Args,
>                                       spy = case lists:keyfind(<<"spy">>, 1,
> Args) of
>                                              false -> false;
>                                              _ -> true
>                                       end,
>                                       exclusive_owner = Owner,
>                                       pid = none}),

I'd probably bring that out to a Spy variable. Also, take a look at
rabbit_misc:table_lookup/2. Also, you might want to delay that
inspection until you're in amqqueue_process:delay - though
unfortunately, because it needs to be in the mnesia record, you'll have
to do it before the call to amqqueue:internal_declare, rather than as
part of process_args. It probably doesn't make too much difference
either way...

> *(edition of rabbit_amqqueue:deliver/2)*
> *
> *
>      deliver(QPid, Delivery = #delivery{immediate = true}) ->
>           gen_server2:call(QPid, {deliver_immediately, Delivery}, infinity);
>      deliver(QPid, Delivery = #delivery{mandatory = true}) ->
>           case gen_server2:call(QPid, {deliver, Delivery}, infinity) of
>                spy -> spy;
>                _ -> true

Purely cosmetic: we'd vertically align the -> arrows there.

I would imagine you're missing a diff here: surely the handle_call in
amqqueue_process for the mandatory delivery that should be returning
true|spy?

> *rabbit_amqqueue_router.erl :*
> *(edition of rabbit_router:fold_deliveries/2, line 105)*
> 
>      fold_deliveries({Pid, true},{_, Handled}) -> {true, [Pid|Handled]};
>      fold_deliveries({_,  spy}, {false, Handled}) -> {false, Handled};
>      fold_deliveries({_,  _},{_, Handled}) -> {true, Handled}.

Neat: so a response of spy causes the delivery to that queue to be
ignored from the pov of mandatory.

This is very impressive for a first hack on Rabbit.

Matthew