Large message code review

[timfox]

Here are the results of the large message code review:

1. Need some tests that show pre-commit acks working with large messages

2. Need some tests show in large messages failover.

3. We should use SessionSendMessage for sending both large and normal messages. We should then have a new packet called SessionSendMessageContinuation for sending extra chunks.

4. We should use SessionReceiveMessage for receiving both large and normal messages, with continuations in a different packet.

5. Chunk messages are currently always sent blocking. Instead only the last chunk should be sent blocking, and then, only if sendBlocking = true

6. Replication for large messages needs to be implemented

7. ClientConsumeImpl::handleChunk should be synchronized and have closing check like handleMessage()

8.ClientConsumerImpl shouldn't use instanceof to determine FileMessage, instead use isFileConsiumer()

9.Flow control - for large message flow control should be called before adding adding to buffer APART from last chunk which should have flow control called before consumption, otherwise could flood the buffer.

[timfox]

Another point:

ClientMessage:isLargeMessage()

ClientMessage:setLargeMessage() is only getting called for file messages, not large messages in general.

Also in ServerConsumerImpl:

Ugly instanceof check:

if (message instanceof ServerLargeMessage)
{

Instead, how about moving isLargeMessage to the Message interface so it's implemented by both ClientMessage and ServerMessage, and it can be specified in the constructor?

[timfox]

***

9.Flow control - for large message flow control should be called before adding adding to buffer APART from last chunk which should have flow control called before consumption, otherwise could flood the buffer.

***

[clebert.suconic]

10 - Cleanup client files on non-consumed-files on the FileConsumer

[timfox]

Reviewing the latest state of trunk, it seems:

1) is done

2) is outstanding

3) is done

4) is done

5) is NOT done:

 sendMessageInChunks(true, msg);

It's always called with true!

6) tests outstanding

7) is done

8) is done

9) Still has issues:

 public synchronized void handleLargeMessage(final byte[] header) throws Exception
 {
 if (closing)
 {
 // This is ok - we just ignore the message
 return;
 }

 // Flow control for the first packet, we will have others
 flowControl(header.length); *** WRONG**

 currentChunkMessage = createFileMessage(header);
 }

The flow control number of bytes must be the packet size which is not the same as the header.length!!

I can also see you're not using the packet size for to reduce the credits in the serverconsumerimpl. Packet size MUST be used.

Flow control size also seems be calculated wrong in other places.

Other issues:

 /** Size used for FlowControl */
 int getFlowControlSize();

 /** Size used for FlowControl */
 void setFlowControlSize(int flowControlSize);

This methods should NOT be on the public API, they are for internal use only, not for use by users!!

Another issue:

ClientMessage::setFlowControlSize is only being called for large messages. So for normal messages it's always zero, meaning no credits are sent to the server............. ouch ouch

not good

[clebert.suconic]

ClientMessage::setFlowControlSize is only being called for large messages. So for normal messages it's always zero, meaning no credits are sent to the server............. ouch ouch

not good

The default value for flowControlSize is getEncodingSize() which is what it used to be.


I'm using the headerSize because on regular messages you are using Message.getEncodeSize() and not Packet.getSize(). I had actually it implemented at packetSize at some point, but I changed it to byteBuffer.length when I saw that.

[clebert.suconic]

I would need to create a ClientMessageInternal interface for that, and I would need to probably change all the usages from ClientMessage to ClientMessageInternal. (Or else I would need to type-cast to ClientMessageInternal).

[timfox]

I would need to create a ClientMessageInternal interface for that, and I would need to probably change all the usages from ClientMessage to ClientMessageInternal. (Or else I would need to type-cast to ClientMessageInternal).

Instead of spending 3 hours debating this, I just made the change myself.

I timed myself, it took 2 minutes 30 second to refactor the code.

It's in SVN now.

[timfox]

I'm thinking of moving some of the other methods from ClientMessage to ClientMessageInternal as well, since they shouldn't really be exposed to the user.

[clebert.suconic]

"timfox" wrote:

I would need to create a ClientMessageInternal interface for that, and I would need to probably change all the usages from ClientMessage to ClientMessageInternal. (Or else I would need to type-cast to ClientMessageInternal).

Instead of spending 3 hours debating this, I just made the change myself.

I timed myself, it took 2 minutes 30 second to refactor the code.

It's in SVN now.

Well.. you don't need to convince yourself :-)


I wanted to double check to verify if you wouldn't disagree with that.

[timfox]

I wanted to double check to verify if you wouldn't disagree with that.

Didn't we already did this on IRC? I was the one who suggested moving them into another interface in the first place. So why do you need to convince me? ;)

[timfox]

Other issues:

1) Method boolean isFileConsumer(); should be on ClientConsumerInternal NOT the public API - why would a user call this?

2) ClientFileMessage -
the methods setLargeMessage,
getChannel, closeChannel - should they really be on the public API??

[timfox]

Ok, that's done in SVN too.

[clebert.suconic]

Everything but clustering is done.

However I just realized that Expiry and DLQ won't work with ChunkMessages. I will review it tomorrow.

[clebert.suconic]

Flow control size also seems be calculated wrong in other places.

I tried using packetSize on the regular message, but jmsTest/ConnectionFactoryTest::slowConsumer hang. I'm keeping getEncodeSize() on the regular message for now.