Bug 507421

Cluster flow control fixed as of r813100

The fix to cluser flow control requires both of the following commits
813100 and 810591

Can term "long runs of calls" be specified with some integer approximation. 

does "credit" in "...decode without any credit..." mean, that line "giveReadCredit(1);" is actually not evaluated within qpid::cluster::Connection::decode?

Actually i've tried to check on described that behaviour with r810573 over gdb, started similar configuration as in bug 507388, and then set up break on line where giveReadCredit(1); is inside qpid::cluster::Connection::decode while running perftest --durable=true --size=8
During my last try i saw 914 calls of qpid::cluster::Connection::decode which never resulted in calling giveReadCredit(1);


otherwise when i checked with just one "break qpid::cluster::Connection::giveReadCredit"
then many calls of giveReadCredit method (1500254) were observed

Can you please clarify this issue?

Long runs means > --cluster-read-max which is 10 by default. I was seeing runs of 20-100 I believe. I demonstrated the problem by adding log or printf statements to giveReadCredit and decode. If flow control is working then this should always be true:

0 <= (number_of_decode - number_of_giveReadCredit) <= --cluster-read-max

Not sure what you were seeing in the debugger.

I'm still not able to see described behaviour, even when I rolled to little bit older version of qpid. 

qpidd-0.5.752581-3.el5
qpidc-rdma-0.5.752581-3.el5
qpidd-acl-0.5.752581-3.el5
qpidd-ssl-0.5.752581-3.el5
qpidc-perftest-0.5.752581-3.el5
qpid-java-qman-0.5.751061-1.el5
python-qpid-0.5.752581-1.el5
qpid-dotnet-0.4.738274-2.el5
qpidc-ssl-0.5.752581-3.el5
ruby-qpid-0.4.749380-2.el5
qpidd-cluster-0.5.752581-3.el5
qpid-java-client-0.5.751061-1.el5
qpidd-devel-0.5.752581-3.el5
qpidc-devel-0.5.752581-3.el5
qpidd-rdma-0.5.752581-3.el5
qpidc-debuginfo-0.5.752581-3.el5
qpidc-0.5.752581-3.el5
qpid-java-common-0.5.751061-1.el5
qpidd-xml-0.5.752581-3.el5

As i don't have such a deep knowledge about whole credit mechanism can you please attach Connection.cpp file containing, as you've said "log or printf statements to giveReadCredit and decode", to show how exactly this issue can be reproduced. I'm using the same reproducer as posted in bug 507388

Hi Alan,
could you possibly specify in more detail how you would like us to test that cluster flow control is working properly, please?
Petr spent some time ago more than week w/o success.

Thanks upfront.

Created attachment 451736 [details]
Patch to verify read-credit on recent code.

Created attachment 451738 [details]
Patch to verify read-credit on revision 752560.

How to verify this bug: Unfortunately I can't think of any way to verify this bug without instrumenting the code so I've attached 2 patches. 

Apply rccheck-752560.patch to revision 752560, start a clustered broker and run 
perftest --size 8 --durable

You should see a lot of debugging messages and an assertion failure: 

qpidd: ../../qpid/cpp/src/qpid/cluster/Connection.cpp:218: size_t qpid::cluster::Connection::decode(const char*, size_t): Assertion `cc >= 0' failed.

Now apply rccheck-trunk.patch to the version you want to verify and run perftest as before. You should see a lot of debugging messages but no assertion failure.

The patches are functionally the same, but the code has changed so much that  separate patch files are needed for the different revisions.

I tried to compile qpid at revision 752560 with patch from attachment to reproduce the issue:

qpid/cluster/Connection.cpp: In member function 'void qpid::cluster::Connection::giveReadCredit(int)':
qpid/cluster/Connection.cpp:101: error: 'class qpid::cluster::Cluster' has no member named 'getSettings'
make[3]: *** [qpid/cluster/Connection.lo] Error 1

void Connection::giveReadCredit(int credit) {
    if (cluster.getReadMax() && credit) {
        // FIXME aconway 2010-10-05: TEST ONLY
        int cc = (readCreditCheck += credit);
        QPID_LOG(critical, "FIXME " << this << " giveReadCredit(" << credit << ") " << cc);
        assert(cc <= int(cluster.getSettings().readMax));

        output.giveReadCredit(credit);
    }
}

The getSettings method is missing in particular Class. Please could you take a look at it, and found the right revision which can be compiled with your patch?

Created attachment 451884 [details]
Correct patch to verify read-credit on revision 752560.

This is the correct patch for revision 752560, previous one was a mistake.

Restested over current packages on RHEL5/x86 and x86_64 with:

qpid-cpp-server-store-0.7.946106-17.el5
qpid-tests-0.7.946106-1.el5
qpid-cpp-server-0.7.946106-17.el5
qpid-cpp-client-devel-0.7.946106-17.el5
python-qpid-0.7.946106-14.el5
qpid-java-common-0.7.946106-10.el5
qpid-cpp-client-devel-docs-0.7.946106-17.el5
qpid-cpp-server-devel-0.7.946106-17.el5
qpid-tools-0.7.946106-11.el5
qpid-cpp-server-xml-0.7.946106-17.el5
qpid-cpp-client-0.7.946106-17.el5
qpid-cpp-server-ssl-0.7.946106-17.el5
qpid-java-client-0.7.946106-10.el5
qpid-cpp-server-cluster-0.7.946106-17.el5
qpid-cpp-mrg-debuginfo-0.7.946106-17.el5
qpid-cpp-client-ssl-0.7.946106-17.el5

No regression found.

>>> VERIFIED

Tech note not required, this problem was internally found & fixed there are no known cases of it causing a user-visible problem.

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHSA-2010-0773.html