[ataylor]

I suggest you just do a clone for now, and get the rest working first. Break it into manageable chunks

do you mean create anther implementation of PriorityLinkedList rather than replace it?

[timfox]

Just keep the implementation as is for now, and clone it to get messages as we currently do.

[ataylor]

Just keep the implementation as is for now, and clone it to get messages as we currently do.

Ok, i'll do this first and get it checked in, then work on the new impl.

[timfox]

I just had a quick look at the latest code just checked in, and I have a few questions:

1) Why are the methods stop, start, restart, and the extra logic around "messagesWaiting", and the extra server consumer start and stop necessary in the ClientBrowserImpl?

Shouldn't the browsing consumer just behave exactly like a normal consumer?

2) On the server side, the method deliver and DeliverRunner class has been added to the the ServerConsumerImpl, which duplicates code already in the QueueImpl.

A simpler solution would have been to take the copied Set of refs from the queue.list() method, and then instantiate a QueueImpl instance on the fly and just have the consumer consume from that, or create a simple class that just iterates the set and get it to call the handle() method of ServerConsumer, then you wouldn't need to duplicate the code.

[ataylor]

1) Why are the methods stop, start, restart, and the extra logic around "messagesWaiting", and the extra server consumer start and stop necessary in the ClientBrowserImpl?

This to stop delivery of messages and wait for any in transit messages are received before restarting with a new iterator. messagesWaiting is so we can figure out if any messages are left to browse.

A simpler solution would have been to take the copied Set of refs from the queue.list() method, and then instantiate a QueueImpl instance on the fly and just have the consumer consume from that, or create a simple class that just iterates the set and get it to call the handle() method of ServerConsumer, then you wouldn't need to duplicate the code.

Ok, I can do it this way if you prefer.

[timfox]

"ataylor" wrote:

1) Why are the methods stop, start, restart, and the extra logic around "messagesWaiting", and the extra server consumer start and stop necessary in the ClientBrowserImpl?

This to stop delivery of messages and wait for any in transit messages are received before restarting with a new iterator.

Why do you need to do that?

messagesWaiting is so we can figure out if any messages are left to browse.

Can't you use the current receiveImmediate() method to do that?

A simpler solution would have been to take the copied Set of refs from the queue.list() method, and then instantiate a QueueImpl instance on the fly and just have the consumer consume from that, or create a simple class that just iterates the set and get it to call the handle() method of ServerConsumer, then you wouldn't need to duplicate the code.

Ok, I can do it this way if you prefer.

Well, I hope you agree it's simpler and avoids code duplication too ;)

[ataylor]

Why do you need to do that?

I don't when i change it the way you suggest.

Can't you use the current receiveImmediate() method to do that?

This wouldn't work in all cases since there may be messages in transit or waiting to be delivered. typically if you create a consumer and call receive Immediate straight away the first message hasn't arrived yet. Also a fast consumer may consume messages quicker than they are delivered internally.

Well, I hope you agree it's simpler and avoids code duplication too ;)

I do agree, i just wish either 1. I'd thought of it, or b. you'd suggested it earlier.

[ataylor]

Ok so I'm implementing it using a queue then theres no change at all to the consumer code. a couple of slight issues are that when i create the new queue i need to copy the references out of the actual queue before allowing the new queue to start receiving messages. Ive added a createQueueCopy method on session to do this. Also since queue browsers don't need the connection(session) to be started I'm adding a flag on consumer to ignore the stopping/starting of the session and start straight away.

[timfox]

Looking at your latest commit, I notice you added a method on the public client session api "createQueueCopy" - why is this necessary?

Whilst, as a temporary measure the queue needs to be copied before browsing, there is no need for this to be exposed on the client or the wireformat, this should just be an internal implementation detail, when creating the browser on the server.

I'm a bit baffled as to why you chose to do it this way.......

[timfox]

There were a few more issues too:

1) The copy queue was being created using the createQueue method which would also bind it in the post office. This is unnecessary - all we want is a simple in memory copy we can browse

2) The queueCopyLock was unnecessary - the list() method on Queue is already synchronized.

3) New references were being created before adding them to the copy queue. Since the browser queue refs would never get acknowledged, this would prevent the messages from ever getting deleted from the journal.

I have fixed the code and committed.

[timfox]

We also need some core integration tests that test the new browsing implementation.

This is a general thing when writing new stuff on core - we need integration tests. As I mentioned before don't worry too much about the easymock tests, they're not so important.

For browsing I would expect to see:

Browsing with and without selectors.
Verifying messages don't get acknowledged
Browsing with multiple browsers on same queue
Verifying calling acknowledge on message does nothing
etc

Replication with browsing is also currently broke, although this will be hard to get it working with the copying technique - we'll have to wait for the iterator for that.

[ataylor]

Looking at your latest commit, I notice you added a method on the public client session api "createQueueCopy" - why is this necessary?

Whilst, as a temporary measure the queue needs to be copied before browsing, there is no need for this to be exposed on the client or the wireformat, this should just be an internal implementation detail, when creating the browser on the server.

Ok I'll change this.

1) The copy queue was being created using the createQueue method which would also bind it in the post office. This is unnecessary - all we want is a simple in memory copy we can browse

This is so any new messages routed to that address are received by the browser. If we're only bothered about whats in a queue at a certain point in time then We don't need to do it.

2) The queueCopyLock was unnecessary - the list() method on Queue is already synchronized.

Again, this was so no new messages were delivered to the queue after the list method was called.

We also need some core integration tests that test the new browsing implementation.

I'm in the process of writing these.