Review Board 1.7.22


Clustered broker crashes in assertion in cluster/ExpiryPolicy.cpp

Review Request #107 - Created Nov. 17, 2010 and submitted

Alan Conway
qpid-2874
Reviewers
qpid
qpid
   QPID-2874 Clustered broker crashes in assertion in cluster/ExpiryPolicy.cpp

    - Added missing lock to ExpiryPolicy
    - 1-N mapping for expiry ID to mapping when receiving an update.
    - Regression test.

    A fan-out message (sent to multiple queues e.g. by fanout or topic
    exchange) is a single message on multiple queues with a single expiry
    ID. During an update however each instance is sent as a separate
    message so we need to allow 1-N mapping of expiry ID to message during
    update.

The problem is to do with messages that are fanned-out to multiple queues.
The cluster update process does not recognize the same message on different
queues and updates as if it were two distinct messages. The cluster expiry code
expects a 1-1 correspondence between messages and expiry-ids, which are
assigned per message, not per-queued-message. 

This patch allows a 1-n correspondence between expiry-id and messages received via update, and ensures that all messages with the same expiry-id are expired at the same time.

Arguably it would be better to resolve the underlying problem - have update recognize the same message on different queues and send the actual message only once and inform the updatee of the queues it is on. That would be a more involved fix, and would be redundant if we implement some form the new cluster design.  This patch fix solves the immediate problem for the shorter term. 

Passing the reproducers, make check and make check-long. See qpid-2874
Posted (Nov. 18, 2010, 9:42 a.m.)
Is the additional locking part of this specific issue? i.e. is it required specifically by the move to moving from 1-1 tp 1-n? Patch looks fine to me, just curious on that point.
  1. Not related to 1-n, but there was a slightly different assertion seen before adding the locking. Also by inspection it's clear that the locks should be there.