qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Pavel Moravec" <pmora...@redhat.com>
Subject Re: Review Request 23305: [C++ broker] Make memory usage consistent after broker restart
Date Thu, 10 Jul 2014 12:38:54 GMT


> 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).
> 
> Alan Conway wrote:
>     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.

Good point with SharedState after restart - I thought journal recovery will resolve the problem
but I was wrong.

The problem with global counter on SharedStore is that it affects all queues and all messages,
so it would have negative impact to memory usage in most cases. Something I tried to avoid
(also by using the per-queue option disabled by default).

I don't see an easy way how to move forward at the moment to fullfil all (already minimized)
requirements - until e.g. adding message annotation does not create a new copy of the message.
So I am putting on hold this (at least for a while).


- 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message