Something wrong with HornetQFrameDecode?

[clebert.suconic]

Hudson seems less stable now, with more failures then what we used to have.

I was doing some investigation then.

I started by looking into MessageHeaderTest::testMessageOrderTopic.

I was reverting to previous changes until I found the commit where it started to fail.


My conclusion was, there is definitely something wrong with the HornetQFrameDecode change.


Look at org.hornetq.jms.tests.message.MessageHeaderTest


Add the usual loop test:

public void testLoop() throws Exception
 {
 for (int i = 0 ; i < 5000; i++)
 {
 System.out.println("<<<<<<<<<<<<<<<<<<<<<< " + i + " >>>>>>>>>>>>>>>>>>>>>");
 testMessageOrderTopic();
 tearDown();
 setUp();
 }
 }

Now, update the svn to r8341 which was before Trustin's (or/and) Tim's changes on the FrameDecoders.

The test loop will always pass.


Now, do the same at r8355:

r8355 | trustin | 2009-11-20 21:07:33 -0600 (Fri, 20 Nov 2009) | 5 lines

Resolved issue: HORNETQ-221 (Refactor HornetQFrameDecoder and FrameDecoder)
* Rewrote HornetQFrameDecoder
** Outperforms previous implementation by eliminating unnecessary memory copy
** Always generates a dynamic buffer

the same test will consistently fail. It always fail before 300 for me.



I also used trunk, with the recent changes on the journal while reverting trustin's changes. It didn't fail.


so, there's definetely something wrong with the commit at r8355. I will be investigating it and talking to Trustin later today

[clebert.suconic]

I have talked to Tim.

Trustin was supposed to commit that decoder on the Optimization's branch. (not on trunk).


So, I have reverted it for now. You guys (Tim and Trustin) will have to replace it later.

[clebert.suconic]

Just for the record, I don't know if you guys want to apply those changes or not...


But NettyConsumerWindowSizeTest was *sometimes* taking 10 minutes to complete if you used the new encoder.

(yes.. I said it right... 10 minutes)


It's working fine after reverting it.

[trustin]

Apologies. That is weird. It seemed to work just fine. I will take a look again soon before Tim merges the branch.

[trustin]

The behavior of the original HornetQFrameDecoder has been changed:

http://fisheye.jboss.org/browse/Hornetq/branches/20-optimisation/src/main/org/hornetq/integration/transports/netty/HornetQFrameDecoder.java?r1=8288&r2=8387&u=3&ignore=&k=

The decoder in the trunk generates a frame without the length field, while Tim's modification includes the length field. That was why my new decoder was not working in trunk.

Which behavior is correct?

[trustin]

So, the new decoder was supposed to work only with the optimization branch and it was checked in to trunk (i.e. wrong branch). It seems to work fine with the optimization branch so far.