[Repost] New ChannelBuffer type proposed
Frederic Bregier
fredbregier at free.fr
Tue Sep 29 05:43:00 EDT 2009
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.
More information about the netty-dev
mailing list