-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19566/
-----------------------------------------------------------
Review request for qpid, Gordon Sim and Kim van der Riet.
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
-----
/trunk/qpid/cpp/src/qpid/broker/Exchange.cpp 1577475
/trunk/qpid/cpp/src/qpid/broker/MessageStore.h 1577475
/trunk/qpid/cpp/src/qpid/broker/MessageStoreModule.h 1577475
/trunk/qpid/cpp/src/qpid/broker/MessageStoreModule.cpp 1577475
/trunk/qpid/cpp/src/qpid/broker/NullMessageStore.h 1577475
/trunk/qpid/cpp/src/qpid/broker/NullMessageStore.cpp 1577475
/trunk/qpid/cpp/src/qpid/legacystore/MessageStoreImpl.h 1577475
/trunk/qpid/cpp/src/qpid/legacystore/MessageStoreImpl.cpp 1577475
/trunk/qpid/cpp/src/qpid/linearstore/MessageStoreImpl.h 1577475
/trunk/qpid/cpp/src/qpid/linearstore/MessageStoreImpl.cpp 1577475
/trunk/qpid/cpp/src/qpid/store/MessageStorePlugin.h 1577475
/trunk/qpid/cpp/src/qpid/store/MessageStorePlugin.cpp 1577475
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
