14 Replies Latest reply on Dec 12, 2008 12:52 AM by Clebert Suconic

    Large message code review

    Tim Fox Master

      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.

        • 1. Re: Large message code review
          Tim Fox Master

          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
            Tim Fox Master

            ***

            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 Master

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

              • 4. Re: Large message code review
                Tim Fox Master

                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 Master

                   

                  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 Master

                    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
                      Tim Fox Master

                       

                      "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
                        Tim Fox Master

                        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 Master

                           

                          "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
                            Tim Fox Master

                             

                            "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
                              Tim Fox Master

                              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
                                Tim Fox Master

                                Ok, that's done in SVN too.

                                • 13. Re: Large message code review
                                  Clebert Suconic Master

                                  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 Master

                                     

                                    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.