-
1. Re: Large message code review
timfox Nov 22, 2008 6:38 AM (in response to 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? -
2. Re: Large message code review
timfox Dec 9, 2008 4:26 AM (in response to 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.
*** -
3. Re: Large message code review
clebert.suconic Dec 9, 2008 5:21 PM (in response to timfox)10 - Cleanup client files on non-consumed-files on the FileConsumer
-
4. Re: Large message code review
timfox Dec 10, 2008 9:03 AM (in response to 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 -
5. Re: Large message code review
clebert.suconic Dec 10, 2008 9:16 AM (in response to timfox)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. -
6. Re: Large message code review
clebert.suconic Dec 10, 2008 12:28 PM (in response to 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).
-
7. Re: Large message code review
timfox Dec 10, 2008 1:14 PM (in response to timfox)"clebert.suconic@jboss.com" 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. -
8. Re: Large message code review
timfox Dec 10, 2008 1:15 PM (in response to 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.
-
9. Re: Large message code review
clebert.suconic Dec 10, 2008 1:43 PM (in response to timfox)"timfox" wrote:
"clebert.suconic@jboss.com" 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. -
10. Re: Large message code review
timfox Dec 10, 2008 1:49 PM (in response to timfox)"clebert.suconic@jboss.com" wrote:
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? ;) -
11. Re: Large message code review
timfox Dec 10, 2008 2:00 PM (in response to 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?? -
12. Re: Large message code review
timfox Dec 10, 2008 2:05 PM (in response to timfox)Ok, that's done in SVN too.
-
13. Re: Large message code review
clebert.suconic Dec 11, 2008 10:57 PM (in response to timfox)Everything but clustering is done.
However I just realized that Expiry and DLQ won't work with ChunkMessages. I will review it tomorrow. -
14. Re: Large message code review
clebert.suconic Dec 12, 2008 12:52 AM (in response to timfox)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.