Consumer with byte credit can get ignored if a large message "eclipses" a small one.

Details

Description

Given: A consumer with byte credit N for a queue with messages Big size > N and Small size < N
The consumer should not be able to consume Big. Now remove Big with a different consumer.
The consumer should now be able to consume Small, but doesn't

The problem in Queue.cpp is twofold:

if a consumer returns "false" from accept, it is assumed to be blocked and is removed from the listener set. It won't be notified of new messages

the queue only notifies when new messages arrive. It does not notify when a blocking message is removed.

This is demonstrated by adding the following to test_credit_flow_bytes in message.py:

Alan Conway
added a comment - 18/Oct/10 19:54 It should be possible to use a test just like the one in the description but adding a new broker to the cluster just after creating the pauper and then consuming from the new broker.

Jonathan Robie
added a comment - 13/Aug/10 22:40 Cleaned up the patch.
Replicating the pauper's list should be straightforward, testing to make sure it's replicated may be more difficult. Is there currently a test for that with listeners?

In a cluster, the paupers list will need to be replicated as part of an update to a new member.

Follow the example of the existing code to replicate listeners:

QueueListener::eachListener() called by cluster::UpdateClient::updateQueueListeners()
which calls cluster::UpdateClient::updateQueueListener()
which calls addQueueListener generated from cpp/xml/cluster.xml <control add-queue-listener>
which sends a control that gets dispatched to cluster::Connection::addQueueListener
which calls QpidListener::addListener()

I think the whole chain needs to be duplicated for pauapers

Finally bump up the cluster::CLUSTER_VERSION number since this is a change in the cluster protocol.

I don't much like the term "paupers" but I can't think of a better term.

Alan Conway
added a comment - 13/Aug/10 20:56 In a cluster, the paupers list will need to be replicated as part of an update to a new member.
Follow the example of the existing code to replicate listeners:
QueueListener::eachListener() called by cluster::UpdateClient::updateQueueListeners()
which calls cluster::UpdateClient::updateQueueListener()
which calls addQueueListener generated from cpp/xml/cluster.xml <control add-queue-listener>
which sends a control that gets dispatched to cluster::Connection::addQueueListener
which calls QpidListener::addListener()
I think the whole chain needs to be duplicated for pauapers
Finally bump up the cluster::CLUSTER_VERSION number since this is a change in the cluster protocol.
I don't much like the term "paupers" but I can't think of a better term.

Jonathan Robie
added a comment - 29/Jul/10 17:54 I'm considering using the "obvious fix" from the previous comment.
Consumers will eventually ack, raising their credit again, fragmentation is likely to be an issue only temporarily.

Queue::consumeNextMessage() returns either CONSUME or CANT_CONSUME, it does not distinguish "can't consume because I have no credit and can't read until I get more credit" from "can't consume because I don't have enough credit for this particular message, but might be able to read a different message".

The obvious fix for this bug would be to distinguish these two cases, and keep the consumer on the listener list if it has some credit, but not enough for a given message. But that fix has a problem: suppose a consumer has 1 byte credit, it remains on the listener list, and must be considered for incoming messages, but it will never receive a message, it just slows down the search for a consumer for a given message. And it's likely that many consumers would reach that state.

Another possible solution: require a minimum level of credit, and kick out consumers that do not have that much credit. But there's no obvious way to determine a reasonable minimum credit.

Another possible solution: use a data structure that makes it possible to select a consumer that has at least N bytes of credit. This would necessarily be slower than just picking the next available consumer, and would require a more complex data structure.

Jonathan Robie
added a comment - 29/Jul/10 17:11 Queue::consumeNextMessage() returns either CONSUME or CANT_CONSUME, it does not distinguish "can't consume because I have no credit and can't read until I get more credit" from "can't consume because I don't have enough credit for this particular message, but might be able to read a different message".
The obvious fix for this bug would be to distinguish these two cases, and keep the consumer on the listener list if it has some credit, but not enough for a given message. But that fix has a problem: suppose a consumer has 1 byte credit, it remains on the listener list, and must be considered for incoming messages, but it will never receive a message, it just slows down the search for a consumer for a given message. And it's likely that many consumers would reach that state.
Another possible solution: require a minimum level of credit, and kick out consumers that do not have that much credit. But there's no obvious way to determine a reasonable minimum credit.
Another possible solution: use a data structure that makes it possible to select a consumer that has at least N bytes of credit. This would necessarily be slower than just picking the next available consumer, and would require a more complex data structure.
None of these possible solutions thrill me. Is there a better one?