Server-side browser refactoring

[jmesnil]

I'm working on https://jira.jboss.org/jira/browse/JBMESSAGING-1437 to have the browser
iterates on the queue rather than working on a snapshot copy of the queue.

First steps was to use LinkedBlockingDeque to provide an iterator on PriorityLinkedList which allows for
concurrent traversals and mutations

Next step is to refactor the server-side to have the browser works on the iterator

How does a browser differ from a simple consumer?
2/ the queue will deliver all messages to the browser but never removed the corresponding references
3/ the queue must not deliver messages to the browser through its distribution policy

Delivering messages to browsers and to regular consumers are two different code paths that can
not occur in the same loop

When a consumer is added to the queue, the queue could check if it is a browser or not.
If it is a browser, it is *not* added to the Distributor.

For now there is a single DeliverRunner for the queue (-> regular code path)
We could add other Runnable for every browser with their own iterators.
When the session is prompted to deliver, we must check if the consumer prompting for delivery is
a browser or not.
if it is a browser, a Runnable will be created with its own iterator and executed. It will deliver all ref to the browser *without removing the ref from the iterator*
if it is not a browser, regular code path applies

When a consumer is removed, we check if there is a Runnable associated to it and stop it if necessary
wdyt?

[timfox]

I think you're making this more complex than it needs to be:

I'd probably do something like the following, let's say you've added a method to get an iterator on the queue, then you just need to write a bit of code that does this (pseudocode)

Iterator iter = queue.iterator();

while (iter.hasNext())
{
 MessageReference ref = iter.next();
 HandleStatus status = consumer.handle(ref);
 if (status == HandleStatus.BUSY)
 {
 break;
 }
}

put that in a Runnable and call it when a) the browser is first created and b) when promptdelivery is called on the consumer.

That's it.

[jmesnil]

I've added code to iterate on the queue when the consumer is a browser.
Bulk of the patch is in ServerConsumerImpl.promptDelivery()

However, there are now failures in LargeMessageTest when a browser is created to browse a queue with large messages.

Clebert, I'm not familiar with this part of the code. Could you have a look at the patch attached to https://jira.jboss.org/jira/browse/JBMESSAGING-1432 to help me determine how to make the browser work for both regular and large messages?

[clebert.suconic]

Jeff, I don' t see any files there?

[jmesnil]

sorry clebert, I pasted the wrong link. the patch is attached to https://jira.jboss.org/jira/browse/JBMESSAGING-1437

[clebert.suconic]

You always start a thread that will deliver to the client from the beggining of the queue.

When you call promptDelivery, you are supposed to resume where you left sending messages. And you are aways resuming from the beggining, what is creating order issues on the test.


You are also starting multiple senders as you receive credits. And that is duplicating sends also.


The server should send messages to the client as long as you have credits, when you are out of credits you should give up sending. Next time promptDelivery is called you should resume the process again.


That is not an exclusive problem on LargeMessages. (It's just that you run out of credits earlier with Large Messages).

You will have several problems with regular messages also if you run a test where you limit your credits.


The way the Consumer used to work was on a copy of a Queue, right?

Why don't you create some sort of QueueIterator that encapsulates the iteration?


Maybe:

class QueueIteratorImpl implements Queue
{
 ... you encapsulate the iteration here to the main Queue
}

And on ServerSessionImpl:

 private void doHandleCreateConsumer(final SessionCreateConsumerMessage packet)
{
....


 if (browseOnly)
 {
 Queue theQueue = new QueueIteratorImpl(binding.getBindable());
 }

This way you won't need to change anything on ServerConsumerImpl, and I believe you won't need to create any threads to perform the delivery.


Just an idea.

[clebert.suconic]

In summary, your code don't know how to resume sending at all.

On the process of resuming send, you should also check if largeMessageDeliverer != null and resume sending a largeMessage.


If you provide some sort of QueueIterator implements Queue as I said you won't need to change a line on the ServerConsumer, what would be much easier to debug.

Writing the thread the way you did may look a shortcut (less code), but this promptDelivery thing was a hard thing to tune up. (I needed some time to get my head into it). If you avoid touching it would be better for you.


Just my 2 cents.. and my idea anyway.

[timfox]

I looked at the patch too, and it doesn't look right.

I guess I didn't explain myself well to Jeff.

I'll chat with Jeff in the morning and try to explain the solution better.