-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19566/
-----------------------------------------------------------
(Updated March 28, 2014, 1:11 p.m.)
Review request for qpid, Gordon Sim and Kim van der Riet.
Changes
-------
Simple patch based on
qpid.msg_sequence = ((int64_t) getBootSequence()) << 47 + exchange.msg_sequence;
turned out that qpid.sequence_counter exchange argument (auxiliary, managed
fully by the Exchange) is not further relevant, so I removed it from source
code.
When testing the patch, I realized it resolves the original problem in message
duplicity halfly only - on the consumer side. It does not affect producer
sending redelivered messages after "broker acquires msg, and dies" scenario.
Such messages have redelivered flag set and (much) higher sequence number
(higher by 1 << 47, approx.). But that is expected.
Asking for patch review if qpid.sequence_counter can't be used elsewhere (or if
some use case can't rely on it).
Bugs: QPID-5642
https://issues.apache.org/jira/browse/QPID-5642
Repository: qpid
Description
-------
Elegant (but not performance optimal) way of patch:
1) In Exchange::PreRoute::PreRoute, update exchange in store (whole entry).
2) The update method is dealed very similarly like
MessageStoreImpl::create(db_ptr db, IdSequence& seq, const
qpid::broker::Persistable& p) method, i.e. calls BufferValue that calls
Exchange::encode.
Here the code can be unified by merging MessageStoreImpl::update
intoMessageStoreImpl::create method where the code almost duplicates.
However I do not see the patch as performance efficient, as with every message
preroute, new qpid::framing::Buffer is filled in Exchange::encode method, data
are copied from it to char* BufferValue::data and even then they are really
written to the BDB. While in fact we just update the only one number in the
Buffer.
I tried to come up with less performance invasive approach (for those
interested, see
https://bugzilla.redhat.com/attachment.cgi?id=877576&action=diff - if you dont
have access there, let me write), that keeps qpid::framing::Buffer for every
durable exchange with sequencing enabled, but it returned (among others) into
the need of changing the way store encodes/decodes Exchange instance (change in
Exchange::encode / decode methods). What would make the broker backward
incompatible.
Is the performance penalty (due to Exchange::encode method called for every
message preroute) acceptable?
Is it worth merging MessageStoreImpl::update intoMessageStoreImpl::create
method?
Diffs (updated)
-----
/trunk/qpid/cpp/src/qpid/broker/Exchange.cpp 1582719
Diff: https://reviews.apache.org/r/19566/diff/
Testing
-------
- reproducer from JIRA verified
- automated tests passed (except for those known to fail due to QPID-5641
(valgrind & legacystore)
Thanks,
Pavel Moravec
