> 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. > > Pavel Moravec wrote: > 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).
You are storing shared-state pointers in the store as persistence-ids. If you shut down the broker with messages in the store and re-start it then you may get new messages with shared-state pointers that have the same value as the persistence-ids of messages already in the store. Also I share Gordons concern about setting persistence-id in the broker. That will work OK with our stores but there are other store implementations (windows) that might set peristence-id to some kind of database identifier - setting them in the broker would break such a store. A global counter could be set on the SharedState when the SharedState is created. You would need to add a new field to the persistence record to put it in the store which might be a compatibility issue (unless there's an unused 64bit slot lying around). Using an AtomicValue would minimise the performance impact but there could still be one, so I would make this something that is enabled only if there actually is a store. Again there's the question of how this would work with the windows stores or other possible store implementations, I'm not sure if there's a safe & compatible way to add such a field. - Alan ----------------------------------------------------------- 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 > >
