----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11009/#review20331 -----------------------------------------------------------
I'm not sure I buy the argument against a 32 bit sequence no (or reusing the 32 bit value already there). You detect dropped messages by gaps in the sequence numbers. You'd have to miss a hell of a lot of messages (4 billion or so) before you could think you'd not missed a message when you had actually missed one, and even then you'd have to be extremely unlucky to miss exactly 2^32 messages. /trunk/qpid/cpp/src/qpid/broker/Queue.h <https://reviews.apache.org/r/11009/#comment41882> Do you actually need a member in Queue when the member in settings will still be accessible as settings.sequencing anyway? /trunk/qpid/cpp/src/qpid/broker/Queue.cpp <https://reviews.apache.org/r/11009/#comment41883> This may not be necessary (as above) /trunk/qpid/cpp/src/qpid/broker/Queue.cpp <https://reviews.apache.org/r/11009/#comment41881> [possibly controversial point] I'd prefer to see this in the initialisation list as: sequencing(settings.sequencing), sequenceNo(settings.sequencing ? 1 : 0), In general in a C++ constructor you should do as much of the initialisation as possible in the initialisation list rather than in the body of the constructor. (but note this while comment may be rendered moot, by other comments above and below) /trunk/qpid/cpp/src/qpid/broker/Queue.cpp <https://reviews.apache.org/r/11009/#comment41884> Why not initialise sequenceNo to 0 unconditionally and use ++sequenceNo here? - Andrew Stitcher On May 8, 2013, 5:45 p.m., Ernie Allen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11009/ > ----------------------------------------------------------- > > (Updated May 8, 2013, 5:45 p.m.) > > > Review request for qpid, Alan Conway, Gordon Sim, and Justin Ross. > > > Description > ------- > > The primary purpose of this is to detect when a ring queue overwrites > messages. The client can examine the sequence number and look for gaps which > indicate that messages have been dropped. > > To enable, create the queue with the qpid.queue_msg_sequence arg: > std::string addr("my-queue;" > " {create:sender, delete:always," > " node: {type: queue, x-declare: {arguments:" > " {qpid.queue_msg_sequence:1, qpid.policy_type:ring, > qpid.max_count:4}}}}"); > > Sender sender = session.createSender(addr); > This enables sequencing when a message is enqueued. Sequence numbers start at > 1 and are unsigned 64 bit numbers. > The exchange level sequencing uses "qpid.msg_sequence" as the key. > > *** Question *** Should queues use the same key to enable sequencing as > exchanges? "qpid.msg_sequence" The page > https://cwiki.apache.org/qpid/qpid-extensions-to-amqp.html indicates that > qpid.msg_sequence is supported when declaring a queue, but that key was never > supported for queues. > > > To get the sequence number: > uint64_t seqNo; > seqNo = response.getProperties()["qpid.queue_msg_sequence"]; > > *** Note *** > I added a uint64 to the queue class. There is an existing message sequence > number for queues. It is a SequenceNumber class. > However, this is a 32 bit number. I was leery of changing that to a 64 bit > number since it is used throughout the broker and not just as a msg counter. > I'm assuming a 32 bit number is not sufficient in this case since "wrapping > around" to zero would complicate the logic of detecting overwritten messages. > > There is a public class variable in Queue.h named std::string seqNoKey; It is > unused. My guess is that it was intended for use with the SequenceNumber > class. Can it be removed? > > This does not address the issue of persisting the message sequence after a > broker restart. This will be a future issue. > > Before committing this, C++ and python tests would need to be added. > > > This addresses bug QPID-4591. > https://issues.apache.org/jira/browse/QPID-4591 > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/broker/Queue.h 1478851 > /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1478851 > /trunk/qpid/cpp/src/qpid/broker/QueueSettings.h 1478851 > /trunk/qpid/cpp/src/qpid/broker/QueueSettings.cpp 1478851 > > Diff: https://reviews.apache.org/r/11009/diff/ > > > Testing > ------- > > See attached testring.cpp and mm (make script) > They create a ring queue that holds 4 messages with qpid.queue_msg_sequence, > sends enough messages to overflow the queue, and then retrieves the messages. > It outputs the message sequence numbers. > > > Thanks, > > Ernie Allen > >
