You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Alan Conway <ac...@redhat.com> on 2011/06/02 22:28:13 UTC

Re: Review Request: QPID-3280: Fixed extra failover update messages on brokers joining the cluster.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------

(Updated 2011-06-02 20:28:13.387392)


Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.


Changes
-------

Fixed a bug that caused inconsistencies in the number of messages on failover-update queues.

Still intermittently failing the ttl_failover_test, with an inconsistency in the number of messages on queue "q". It appears that in some cases messages are being expired by the queue cleaner on one broker but by consume on another. I'm chasing this down, if anyone has any insights let me know.


Summary (updated)
-------

QPID-3280: Fixed extra failover update messages on brokers joining the cluster.


QPID-3280: When sending a large number of messages with nonzero TTLs to a cluster, overall message throughput drops by around 20-30% compared to messages with TTL 0.

Replaced the complicated message expirly logic in the cluster with a simpler "cluster clock" for expiry of messages with TTL.


Diffs (updated)
-----

  /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1128070 
  /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1128070 
  /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1128070 
  /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070 
  /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1128070 
  /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1128070 
  /trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1128070 
  /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1128070 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1128070 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1128070 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1128070 
  /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1128070 
  /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1128070 
  /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1128070 
  /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1128070 
  /trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.h 1128070 
  /trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.cpp 1128070 
  /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1128070 
  /trunk/qpid/cpp/src/qpid/sys/Timer.h 1128070 
  /trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1128070 
  /trunk/qpid/cpp/src/tests/QueueTest.cpp 1128070 
  /trunk/qpid/cpp/src/tests/cluster_test_logs.py 1128070 
  /trunk/qpid/cpp/src/tests/cluster_tests.py 1128070 
  /trunk/qpid/cpp/xml/cluster.xml 1128070 

Diff: https://reviews.apache.org/r/791/diff


Testing
-------


Thanks,

Alan


Re: Review Request: QPID-3280: Performance problem with TTL messages.

Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------

(Updated 2011-06-10 21:18:44.812882)


Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.


Changes
-------

Hopefully the last patch in this series.


Summary (updated)
-------

QPID-3280: Performance problem with TTL messages.

When sending a large number of messages with nonzero TTLs to a
cluster, overall message throughput drops by around 20-30% compared to
messages with TTL 0.

The previous approach to TTL in the cluster is replaced with a simpler
"cluster clock". Also QueueCleaner is executed in the cluster timer,
and modified to be deterministic in a cluster.


Diffs (updated)
-----

  /trunk/qpid/cpp/include/qpid/framing/FieldTable.h 1134238 
  /trunk/qpid/cpp/src/CMakeLists.txt 1134238 
  /trunk/qpid/cpp/src/Makefile.am 1134238 
  /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1134238 
  /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1134238 
  /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1134238 
  /trunk/qpid/cpp/src/qpid/broker/Message.h 1134238 
  /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1134238 
  /trunk/qpid/cpp/src/qpid/broker/Queue.h 1134238 
  /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1134238 
  /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1134238 
  /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1134238 
  /trunk/qpid/cpp/src/qpid/broker/RateTracker.h 1134238 
  /trunk/qpid/cpp/src/qpid/broker/RateTracker.cpp 1134238 
  /trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1134238 
  /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1134238 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1134238 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1134238 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1134238 
  /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1134238 
  /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1134238 
  /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1134238 
  /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1134238 
  /trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.h 1134238 
  /trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.cpp 1134238 
  /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1134238 
  /trunk/qpid/cpp/src/qpid/framing/AMQHeaderBody.h 1134238 
  /trunk/qpid/cpp/src/qpid/sys/Timer.h 1134238 
  /trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1134238 
  /trunk/qpid/cpp/src/tests/QueueTest.cpp 1134238 
  /trunk/qpid/cpp/src/tests/cluster_test_logs.py 1134238 
  /trunk/qpid/cpp/src/tests/cluster_tests.py 1134238 
  /trunk/qpid/cpp/xml/cluster.xml 1134238 

Diff: https://reviews.apache.org/r/791/diff


Testing
-------


Thanks,

Alan


Re: Review Request: WIP: Misc fixes to cluster TTL

Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------

(Updated 2011-06-08 17:47:17.142625)


Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.


Changes
-------

I've made a couple more fixes and rebased to latest trunk. There are 2 issues with the patch:
- in repeated runs (5-6) I've seen test_ttl_failover  and test_failover fail with a broker exiting with nothing abnormal in the logs and no core file. 
- performance is about 15% better for messages with TTL, but 10% worse for messages without TTL. 

I'm a bit stuck on the test failures, any help appreciated. I think we probably have to optimize the non-TTL case a bit more.


Summary (updated)
-------

WIP: Misc fixes to cluster TTL

- Fix issue with empty message-properties being added to messages.
- Remove queue cleaner use of system clock
- Fix test bugs

WIP: Use atomic counter and period in place of RateTracker for QueueCleaner.

RateTracker was using the non-cluster clock to decide to run purges or not.
This is a clock-free solution that gives the same result and is safe in cluster.

WIP: agoldste's expiration replication fix.


QPID-3280: Fixed extra failover update messages on brokers joining the cluster.


QPID-3280: When sending a large number of messages with nonzero TTLs to a cluster, overall message throughput drops by around 20-30% compared to messages with TTL 0.

Replaced the complicated message expirly logic in the cluster with a simpler "cluster clock" for expiry of messages with TTL.


Diffs (updated)
-----

  /trunk/qpid/cpp/include/qpid/framing/FieldTable.h 1133166 
  /trunk/qpid/cpp/src/CMakeLists.txt 1133166 
  /trunk/qpid/cpp/src/Makefile.am 1133166 
  /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1133166 
  /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1133166 
  /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1133166 
  /trunk/qpid/cpp/src/qpid/broker/Message.h 1133166 
  /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1133166 
  /trunk/qpid/cpp/src/qpid/broker/Queue.h 1133166 
  /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1133166 
  /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1133166 
  /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1133166 
  /trunk/qpid/cpp/src/qpid/broker/RateTracker.h 1133166 
  /trunk/qpid/cpp/src/qpid/broker/RateTracker.cpp 1133166 
  /trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1133166 
  /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1133166 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1133166 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1133166 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1133166 
  /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1133166 
  /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1133166 
  /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1133166 
  /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1133166 
  /trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.h 1133166 
  /trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.cpp 1133166 
  /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1133166 
  /trunk/qpid/cpp/src/qpid/framing/AMQHeaderBody.h 1133166 
  /trunk/qpid/cpp/src/qpid/sys/Timer.h 1133166 
  /trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1133166 
  /trunk/qpid/cpp/src/tests/QueueTest.cpp 1133166 
  /trunk/qpid/cpp/src/tests/cluster_test_logs.py 1133166 
  /trunk/qpid/cpp/src/tests/cluster_tests.py 1133166 
  /trunk/qpid/cpp/xml/cluster.xml 1133166 

Diff: https://reviews.apache.org/r/791/diff


Testing
-------


Thanks,

Alan


Re: Review Request: WIP: Fix issue with empty message-properties being added to messages.

Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------

(Updated 2011-06-03 21:43:43.083444)


Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.


Changes
-------

I think this might be the Last Patch.


Summary (updated)
-------

WIP: Fix issue with empty message-properties being added to messages.


WIP: Use atomic counter and period in place of RateTracker for QueueCleaner.

RateTracker was using the non-cluster clock to decide to run purges or not.
This is a clock-free solution that gives the same result and is safe in cluster.

WIP: agoldste's expiration replication fix.


QPID-3280: Fixed extra failover update messages on brokers joining the cluster.


QPID-3280: When sending a large number of messages with nonzero TTLs to a cluster, overall message throughput drops by around 20-30% compared to messages with TTL 0.

Replaced the complicated message expirly logic in the cluster with a simpler "cluster clock" for expiry of messages with TTL.


Diffs (updated)
-----

  /trunk/qpid/cpp/include/qpid/framing/FieldTable.h 1131084 
  /trunk/qpid/cpp/managementgen/Makefile.am 1128070 
  /trunk/qpid/cpp/src/CMakeLists.txt 1131084 
  /trunk/qpid/cpp/src/Makefile.am 1131084 
  /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/broker/Message.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070 
  /trunk/qpid/cpp/src/qpid/broker/Queue.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/broker/RateTracker.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/RateTracker.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/framing/AMQHeaderBody.h 1131084 
  /trunk/qpid/cpp/src/qpid/sys/Timer.h 1131084 
  /trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1131084 
  /trunk/qpid/cpp/src/tests/QueueTest.cpp 1131084 
  /trunk/qpid/cpp/src/tests/cluster_test_logs.py 1131084 
  /trunk/qpid/cpp/src/tests/cluster_tests.py 1131084 
  /trunk/qpid/cpp/xml/cluster.xml 1131084 

Diff: https://reviews.apache.org/r/791/diff


Testing
-------


Thanks,

Alan


Re: Review Request: QPID-3280: Efficient handling of TTL by cluster

Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------

(Updated 2011-06-03 20:30:03.292570)


Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.


Summary (updated)
-------

QPID-3280: Efficient handling of TTL by cluster

WIP: Use atomic counter and period in place of RateTracker for QueueCleaner.

RateTracker was using the non-cluster clock to decide to run purges or not.
This is a clock-free solution that gives the same result and is safe in cluster.

WIP: agoldste's expiration replication fix.


QPID-3280: Fixed extra failover update messages on brokers joining the cluster.


QPID-3280: When sending a large number of messages with nonzero TTLs to a cluster, overall message throughput drops by around 20-30% compared to messages with TTL 0.

Replaced the complicated message expirly logic in the cluster with a simpler "cluster clock" for expiry of messages with TTL.


Diffs
-----

  /trunk/qpid/cpp/managementgen/Makefile.am 1128070 
  /trunk/qpid/cpp/src/CMakeLists.txt 1131084 
  /trunk/qpid/cpp/src/Makefile.am 1131084 
  /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/broker/Message.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070 
  /trunk/qpid/cpp/src/qpid/broker/Queue.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/broker/RateTracker.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/RateTracker.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/sys/Timer.h 1131084 
  /trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1131084 
  /trunk/qpid/cpp/src/tests/QueueTest.cpp 1131084 
  /trunk/qpid/cpp/src/tests/cluster_test_logs.py 1131084 
  /trunk/qpid/cpp/src/tests/cluster_tests.py 1131084 
  /trunk/qpid/cpp/xml/cluster.xml 1131084 

Diff: https://reviews.apache.org/r/791/diff


Testing
-------


Thanks,

Alan


Re: Review Request: WIP: Use atomic counter and period in place of RateTracker for QueueCleaner.

Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/791/
-----------------------------------------------------------

(Updated 2011-06-03 20:29:32.765294)


Review request for qpid, Andrew Stitcher, Alan Conway, Gordon Sim, and Andy Goldstein.


Changes
-------


This patch has the improvements below. It is passing the ttl test but failing 2 other tests in make check. I suspect the extra header is modifying the message size somewhere. The failures are: 
- cluster_tests.ShortTests.test_store_direct_update_match
- It is passing the ttl test but failing 2 other tests in make check. 


Summary (updated)
-------

QPID-3280: Efficient handling of TTL by cluster

WIP: Use atomic counter and period in place of RateTracker for QueueCleaner.

RateTracker was using the non-cluster clock to decide to run purges or not.
This is a clock-free solution that gives the same result and is safe in cluster.

WIP: agoldste's expiration replication fix.


QPID-3280: Fixed extra failover update messages on brokers joining the cluster.


QPID-3280: When sending a large number of messages with nonzero TTLs to a cluster, overall message throughput drops by around 20-30% compared to messages with TTL 0.

Replaced the complicated message expirly logic in the cluster with a simpler "cluster clock" for expiry of messages with TTL.


Diffs (updated)
-----

  /trunk/qpid/cpp/managementgen/Makefile.am 1128070 
  /trunk/qpid/cpp/src/CMakeLists.txt 1131084 
  /trunk/qpid/cpp/src/Makefile.am 1131084 
  /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/ExpiryPolicy.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/broker/Message.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1128070 
  /trunk/qpid/cpp/src/qpid/broker/Queue.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/QueueCleaner.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/broker/RateTracker.h 1131084 
  /trunk/qpid/cpp/src/qpid/broker/RateTracker.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/Cluster.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterPlugin.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterSettings.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ClusterTimer.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/ExpiryPolicy.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.h 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/FailoverExchange.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1131084 
  /trunk/qpid/cpp/src/qpid/sys/Timer.h 1131084 
  /trunk/qpid/cpp/src/qpid/sys/Timer.cpp 1131084 
  /trunk/qpid/cpp/src/tests/QueueTest.cpp 1131084 
  /trunk/qpid/cpp/src/tests/cluster_test_logs.py 1131084 
  /trunk/qpid/cpp/src/tests/cluster_tests.py 1131084 
  /trunk/qpid/cpp/xml/cluster.xml 1131084 

Diff: https://reviews.apache.org/r/791/diff


Testing
-------


Thanks,

Alan