[Repost] New ChannelBuffer type proposed
Frederic Bregier
fredbregier at free.fr
Wed Sep 30 01:56:29 EDT 2009
Hi Trustin,
I suppose that the current fix take your time (I understand), so as I said
that I can have the time the evening, I made it ... this morning before
going at work... ;-)
I test it again with JUnit and it continues to be OK.
So you don't have to patch it before merging.
Cheers,
Frederic
Trustin Lee wrote:
>
> No problem. I will take care of it when I merge. It's not a big deal. :)
>
> — Trustin Lee, http://gleamynode.net/
>
>
>
> On Tue, Sep 29, 2009 at 6:43 PM, Frederic Bregier <fredbregier at free.fr>
> wrote:
>>
>> No, I'm not so fast, but I spend some times one Netty those days ("before
>> production testing" is starting those days).
>> However, I'm not able to make the change into the branch except this
>> evening
>> (no access from my job except mailing list). So do you mind if you take
>> care
>> of those changes ?
>>
>>
>> On the http2 step, OK, I will wait for the 3.2.0.ALPHA1 in trunk in order
>> to
>> have the correct dependencies with the new branch.
>>
>> Cheers,
>> Frederic
>>
>>
>> Trustin Lee wrote:
>>>
>>> The new patch looks great. You are fast :)
>>>
>>> Sure. Please feel free to create another branch as this branch is
>>> going to be merged pretty soon.
>>>
>>> Cheers!
>>>
>>> — Trustin Lee, http://gleamynode.net/
>>>
>>>
>>>
>>> On Tue, Sep 29, 2009 at 6:26 PM, Frederic Bregier <fredbregier at free.fr>
>>> wrote:
>>>>
>>>> Hi Trustin,
>>>>
>>>> Yes the simple wrappefBuffer(ChannelBuffer) (only with one
>>>> ChannelBuffer
>>>> argument is concerned) returns a CompositeChannelBuffer. I was feeling
>>>> that
>>>> it could be better than the default slice which was the default:
>>>>
>>>> @@ -306,7 +306,8 @@
>>>> */
>>>> public static ChannelBuffer wrappedBuffer(ChannelBuffer buffer) {
>>>> if (buffer.readable()) {
>>>> - return buffer.slice();
>>>> + ChannelBuffer[] buffers = { buffer };
>>>> + return new CompositeChannelBuffer(buffers);
>>>> } else {
>>>> return EMPTY_BUFFER;
>>>> }
>>>>
>>>> I see one reason to keep this: if the given ChannelBuffer is itself a
>>>> CompositeChannelBuffer, the result is optimized (less memory since some
>>>> part
>>>> can be removed into the wrapped one) and still a
>>>> CompositeChannelBuffer.
>>>>
>>>> However, if not a CompositeChannelBuffer, I agree that it could be
>>>> simply
>>>> a
>>>> buffer.slice().
>>>>
>>>> Keeping this prevents some bad composition like:
>>>> wrapped(wrapped(wrapped(wrapped(buffer1), buffer2)))
>>>> giving -> [slice(slice((slice(buffer1),buffer2)))]
>>>> but giving -> [slice(buffer1),buffer2]
>>>>
>>>>
>>>> So perhaps something like:
>>>>
>>>> public static ChannelBuffer wrappedBuffer(ChannelBuffer buffer) {
>>>> if (buffer.readable()) {
>>>> - return buffer.slice();
>>>> + if (buffer instanceof CompositeChannelBuffer) {
>>>> + ChannelBuffer[] buffers = { buffer };
>>>> + return new CompositeChannelBuffer(buffers);
>>>> + } else {
>>>> + return buffer.slice();
>>>> + }
>>>> } else {
>>>> return EMPTY_BUFFER;
>>>> }
>>>>
>>>> But then perhaps revert the following change:
>>>> @@ -363,7 +364,7 @@
>>>> break;
>>>> case 1:
>>>> if (buffers[0].readable()) {
>>>> - return wrappedBuffer(buffers[0]);
>>>> + return new CompositeChannelBuffer(buffers);
>>>> }
>>>> break;
>>>> default:
>>>>
>>>> WDYT ?
>>>>
>>>>
>>>> Also, would you like that I create too another branch for the proposed
>>>> http2
>>>> renamed in http (body post encoder and decoder) that was related to
>>>> this
>>>> work too ?
>>>>
>>>> Cheers,
>>>> Frederic
>>>>
>>>>
>>>> Trustin Lee wrote:
>>>>>
>>>>> I see a problem with ChannelBuffers.wrappedBuffer(). It's not a big
>>>>> problem as we can fix it easily though. The problem is that
>>>>> ChannelBuffers.wrappedBuffer(ChannelBuffer) will create a
>>>>> CompositeChannelBuffer even when the specified buffer is not a
>>>>> composite buffer.
>>>>>
>>>>> Due to the latest regression I've put into 3.1.4, I think I have to
>>>>> release 3.1.5 soon. And then I will merge your work to the trunk and
>>>>> proceed to 3.2.0.ALPHA1.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> — Trustin Lee, http://gleamynode.net/
>>>>>
>>>>>
>>>>>
>>>>> On Sun, Sep 27, 2009 at 4:33 PM, Frederic Bregier
>>>>> <fredbregier at free.fr>
>>>>> wrote:
>>>>>>
>>>>>> Hi Trustin,
>>>>>>
>>>>>> I've just create the "aggregate" branch. It includes the modified
>>>>>> CompositeChannelBuffer, ChannelBuffers (correction of wrapped
>>>>>> methods)
>>>>>> ,
>>>>>> and
>>>>>> the extension of the existing two JUnit test case (BigEndianComposite
>>>>>> and
>>>>>> LittleEndianComposite ChannelBufferTest).
>>>>>>
>>>>>> I've rerun the Junit test and all seem ok.
>>>>>>
>>>>>> Cheers,
>>>>>> Frederic
>>>>>>
>>>>>>
>>>>>> Frederic Bregier wrote:
>>>>>>>
>>>>>>> Hi Trustin,
>>>>>>>
>>>>>>> Ok with the new branch.
>>>>>>> I will create it named "aggregate" in order to know for what reason
>>>>>>> it
>>>>>>> is
>>>>>>> there, if you're ok.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Frederic
>>>>>>>
>>>>>>>
>>>>>>> Trustin Lee wrote:
>>>>>>>>
>>>>>>>> Hi Frederic,
>>>>>>>>
>>>>>>>> On Fri, Sep 25, 2009 at 2:14 PM, Frederic Bregier
>>>>>>>> <fredbregier at free.fr>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> Hi Trustin,
>>>>>>>>>
>>>>>>>>> I'm agree that we could replace CompositeChannelBuffer with this
>>>>>>>>> one
>>>>>>>>> (so
>>>>>>>>> not
>>>>>>>>> introducing a new name).
>>>>>>>>> I had done like this in order to allow anyone to test it without
>>>>>>>>> removing
>>>>>>>>> CCB from Netty.
>>>>>>>>> Now that you think it is ok, I will propose you the new code for
>>>>>>>>> CCB
>>>>>>>>> using
>>>>>>>>> the one from the Aggregate.
>>>>>>>>>
>>>>>>>>> For wrappedCheckedBuffer, in fact, it was strictly in the same
>>>>>>>>> mind
>>>>>>>>> that
>>>>>>>>> for
>>>>>>>>> CCB, that is to say that I did not wanted to remove the default
>>>>>>>>> wrappedBuffer from ChannelBuffers. But of course, I would propose
>>>>>>>>> you
>>>>>>>>> the
>>>>>>>>> patch for ChannelBuffers such that wrappedBuffer take care of this
>>>>>>>>> without
>>>>>>>>> introducing any new method...
>>>>>>>>>
>>>>>>>>> Give me a day or two (at least before the end of the week-end),
>>>>>>>>> and
>>>>>>>>> I
>>>>>>>>> will
>>>>>>>>> post a new set of java code (replacement for
>>>>>>>>> CompositeChannelBuffer
>>>>>>>>> and
>>>>>>>>> its
>>>>>>>>> Junit test, and the patch for ChannelBuffers).
>>>>>>>>> Are you ok with this ?
>>>>>>>>
>>>>>>>> Sure. Sounds great!
>>>>>>>>
>>>>>>>>> I could also directly patch on trunk but if you don't mind, I
>>>>>>>>> would
>>>>>>>>> prefer
>>>>>>>>> you check a last time before comiting, ok ?
>>>>>>>>
>>>>>>>> Let's create a new temporary branch. I still need to maintain
>>>>>>>> 3.1.x
>>>>>>>> branch and this feature should go to 3.2. WDYT?
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>> — Trustin Lee, http://gleamynode.net/
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>> Frederic
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Trustin Lee wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Frederic,
>>>>>>>>>>
>>>>>>>>>> The code looks OK, but I'm still not sure if we need to make
>>>>>>>>>> wrappedCheckedBuffer() public. We could modify
>>>>>>>>>> ChannelBuffers.wrappedBuffer() to recognize composite buffers
>>>>>>>>>> using
>>>>>>>>>> instanceof.
>>>>>>>>>>
>>>>>>>>>> Also, if your implementation works in the same way with
>>>>>>>>>> CompositeChannelBuffer, then we could simply replace it with your
>>>>>>>>>> buffer type and rename it to CompositeChannelBuffer, rather than
>>>>>>>>>> introducing a new name?
>>>>>>>>>>
>>>>>>>>>> Thanks for your patience
>>>>>>>>>>
>>>>>>>>>> — Trustin Lee, http://gleamynode.net/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Thu, Sep 10, 2009 at 5:13 AM, Frederic Bregier
>>>>>>>>>> <fredbregier at free.fr>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi Trustin,
>>>>>>>>>>>
>>>>>>>>>>> I've found a bug in the slice implementation when start is not
>>>>>>>>>>> at
>>>>>>>>>>> 0.
>>>>>>>>>>> So I correct this and add a last line in the
>>>>>>>>>>> testDiscardReadBytes
>>>>>>>>>>> method
>>>>>>>>>>> of
>>>>>>>>>>> the Junit test (in Aggregate since it override the default one
>>>>>>>>>>> from
>>>>>>>>>>> Abstract
>>>>>>>>>>> Junit test since this original test could be buggy for the test
>>>>>>>>>>> of
>>>>>>>>>>> not
>>>>>>>>>>> equality of extra bytes as I wrote in previous mail).
>>>>>>>>>>>
>>>>>>>>>>> Here are the two files:
>>>>>>>>>>> http://n2.nabble.com/file/n3613619/AggregateChannelBuffer.java
>>>>>>>>>>> AggregateChannelBuffer.java
>>>>>>>>>>> http://n2.nabble.com/file/n3613619/AggregateChannelBufferTest.java
>>>>>>>>>>> AggregateChannelBufferTest.java
>>>>>>>>>>>
>>>>>>>>>>> Cheers,
>>>>>>>>>>> Frederic
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Frederic Bregier wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Trustin,
>>>>>>>>>>>>
>>>>>>>>>>>> Ok I write and test the slice functionality (using Junit).
>>>>>>>>>>>> It seems OK.
>>>>>>>>>>>>
>>>>>>>>>>>> I still feel like writeBytes should be left as the default
>>>>>>>>>>>> implementation
>>>>>>>>>>>> (ie the one from AbstractChannelBuffer) since the setBytes
>>>>>>>>>>>> method
>>>>>>>>>>>> should
>>>>>>>>>>>> do the trick as usual.
>>>>>>>>>>>> I test by using the default implementation, and all tests pass
>>>>>>>>>>>> too...
>>>>>>>>>>>> So, except if you think that writing by slice from source could
>>>>>>>>>>>> be
>>>>>>>>>>>> useful
>>>>>>>>>>>> (if the source is an aggregate one), I think this method could
>>>>>>>>>>>> be
>>>>>>>>>>>> removed
>>>>>>>>>>>> from Aggregate (so not implementing in the modified version of
>>>>>>>>>>>> Component
>>>>>>>>>>>> if ok) so letting the default one be called.
>>>>>>>>>>>> I put a comment on this method.
>>>>>>>>>>>>
>>>>>>>>>>>> Cheers,
>>>>>>>>>>>> Frederic
>>>>>>>>>>>>
>>>>>>>>>>>> http://n2.nabble.com/file/n3565134/AggregateChannelBuffer.java
>>>>>>>>>>>> AggregateChannelBuffer.java
>>>>>>>>>>>> http://n2.nabble.com/file/n3565134/AggregateChannelBufferTest.java
>>>>>>>>>>>> AggregateChannelBufferTest.java
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> -----
>>>>>>>>>>> Hardware/Software Architect
>>>>>>>>>>> --
>>>>>>>>>>> View this message in context:
>>>>>>>>>>> http://n2.nabble.com/New-ChannelBuffer-type-proposed-tp3496606p3613619.html
>>>>>>>>>>> Sent from the Netty Developer Group mailing list archive at
>>>>>>>>>>> Nabble.com.
>>>>>>>>>>> _______________________________________________
>>>>>>>>>>> netty-dev mailing list
>>>>>>>>>>> netty-dev at lists.jboss.org
>>>>>>>>>>> https://lists.jboss.org/mailman/listinfo/netty-dev
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> _______________________________________________
>>>>>>>>>> netty-dev mailing list
>>>>>>>>>> netty-dev at lists.jboss.org
>>>>>>>>>> https://lists.jboss.org/mailman/listinfo/netty-dev
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> -----
>>>>>>>>> Hardware/Software Architect
>>>>>>>>> --
>>>>>>>>> View this message in context:
>>>>>>>>> http://n2.nabble.com/New-ChannelBuffer-type-proposed-tp3496606p3710508.html
>>>>>>>>> Sent from the Netty Developer Group mailing list archive at
>>>>>>>>> Nabble.com.
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> netty-dev mailing list
>>>>>>>>> netty-dev at lists.jboss.org
>>>>>>>>> https://lists.jboss.org/mailman/listinfo/netty-dev
>>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> netty-dev mailing list
>>>>>>>> netty-dev at lists.jboss.org
>>>>>>>> https://lists.jboss.org/mailman/listinfo/netty-dev
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> -----
>>>>>> Hardware/Software Architect
>>>>>> --
>>>>>> View this message in context:
>>>>>> http://n2.nabble.com/New-ChannelBuffer-type-proposed-tp3496606p3719936.html
>>>>>> Sent from the Netty Developer Group mailing list archive at
>>>>>> Nabble.com.
>>>>>>
>>>>>> _______________________________________________
>>>>>> netty-dev mailing list
>>>>>> netty-dev at lists.jboss.org
>>>>>> https://lists.jboss.org/mailman/listinfo/netty-dev
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> netty-dev mailing list
>>>>> netty-dev at lists.jboss.org
>>>>> https://lists.jboss.org/mailman/listinfo/netty-dev
>>>>>
>>>>>
>>>>
>>>>
>>>> -----
>>>> Hardware/Software Architect
>>>> --
>>>> View this message in context:
>>>> http://n2.nabble.com/New-ChannelBuffer-type-proposed-tp3496606p3734901.html
>>>> Sent from the Netty Developer Group mailing list archive at Nabble.com.
>>>>
>>>> _______________________________________________
>>>> netty-dev mailing list
>>>> netty-dev at lists.jboss.org
>>>> https://lists.jboss.org/mailman/listinfo/netty-dev
>>>>
>>>
>>> _______________________________________________
>>> netty-dev mailing list
>>> netty-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/netty-dev
>>>
>>>
>>
>>
>> -----
>> Hardware/Software Architect
>> --
>> View this message in context:
>> http://n2.nabble.com/New-ChannelBuffer-type-proposed-tp3496606p3734984.html
>> Sent from the Netty Developer Group mailing list archive at Nabble.com.
>>
>> _______________________________________________
>> netty-dev mailing list
>> netty-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/netty-dev
>>
>
> _______________________________________________
> netty-dev mailing list
> netty-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/netty-dev
>
>
-----
Hardware/Software Architect
--
View this message in context: http://n2.nabble.com/New-ChannelBuffer-type-proposed-tp3496606p3740900.html
Sent from the Netty Developer Group mailing list archive at Nabble.com.
More information about the netty-dev
mailing list