> On July 8, 2014, 1:55 p.m., Alan Conway wrote: > > Using a pointer as an identifier isn't safe. Pointer values can be re-used > > as soon as the in-memory copy of the message is deleted, so they are not > > unique over time. A global atomic counter might be a possibility but we > > would need to benchmark for performance consequences before making this the > > default behavior.
The pointer to SharedState should become invalid once all copies of the message are gone. I.e. once the messages disappear from store as well. Even then the pointer can be re-used for some another object, optionally another SharedState ptr. (until we use paged queues but there is a check the option can't be used together with paged queues). Using global atomic counter: how to set the same counter value to two copies of the same message (with the same SharedState) being enqueued to different queues? Or, re-phrasing the question, how to identify two messages enqueued to two different queues are just two instances of the same message (with the same SharedState)? That is the crucial question here. The pointer to SharedState is easy solution, though I agree that in general pointers should not be used as some (unique) reference IDs. Honestly, I see my solution somehow "fragile" but can't transform that feeling into any particular technical objection against the patch (and I thought a lot what all could it break). - Pavel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23305/#review47442 ----------------------------------------------------------- On July 8, 2014, 12:53 p.m., Pavel Moravec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23305/ > ----------------------------------------------------------- > > (Updated July 8, 2014, 12:53 p.m.) > > > Review request for qpid, Gordon Sim and Kim van der Riet. > > > Bugs: QPID-5880 > https://issues.apache.org/jira/browse/QPID-5880 > > > Repository: qpid > > > Description > ------- > > Simple idea: > - in Queue::enqueue, set PersistableMessage::persistencyID "manually" to some > unique number that is identical to all message instances that has common > SharedState - e.g. to the pointer to SharedState > - during journal recovery, if we recover a message with already seen > persistencyID, use the previous seen instead with its SharedState and > PersistableMessage bits > > Known limitation: > - message annotations added to some queue (e.g. due to queue sequencing > enabled) will be either over-written or shared to all other queues during > recovery > > The patch contains a new QueueSettings option to enable (by default disabled) > this feature on per queue basis. This somehow limits the limitation above. > > Isn't storing pointer to SharedState to the disk (via persistencyID) some > sort of security breach? (I dont think so, but worth to ask) > > Can't manual setup of persistencyID break something in store? (AFAIK no as > uniqueness of the ID is assured: 1) a new / different message with the same > persistencyID can appear only after the previous instance is gone from > memory, and 2) only queues with the option enabled are checked for message > coupling) > > Will it help in cluster? No, it won't. As when primary broker gets 1 message > to an exchange that distributes it to 100 queues, th broker updates backup > brokers via 100 individual "enqueue 1 message to queue q[1-100]" events. So > backup brokers consume more memory than primary - the same amount like > primary does not share SharedState at all. > > So it is reasonable for standalone brokers only. > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1608083 > /trunk/qpid/cpp/src/qpid/broker/QueueSettings.h 1608083 > /trunk/qpid/cpp/src/qpid/broker/QueueSettings.cpp 1608083 > /trunk/qpid/cpp/src/qpid/legacystore/MessageStoreImpl.h 1608083 > /trunk/qpid/cpp/src/qpid/legacystore/MessageStoreImpl.cpp 1608083 > /trunk/qpid/cpp/src/qpid/linearstore/MessageStoreImpl.h 1608083 > /trunk/qpid/cpp/src/qpid/linearstore/MessageStoreImpl.cpp 1608083 > > Diff: https://reviews.apache.org/r/23305/diff/ > > > Testing > ------- > > No significant difference in memory consumption before & after restart (in > setup of 500 queues with qpid.store_msgID=true and thousands of messages sent > via fanout exchange to all of them). > > Automated tests passed. > > > Thanks, > > Pavel Moravec > >
