[
https://issues.apache.org/jira/browse/THRIFT-1035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12980406#action_12980406
]
Mathias Herberts commented on THRIFT-1035:
------------------------------------------
I understand the performance benefit when the returned ByteBuffers are slices
of the underlying buffer, but don't you feel this exposes internal structures
too much?
With this approach you can access the array backing the transport's buffer and
modify it without you knowing it (i.e. a user simply using the returned
ByteBuffer), which means that modifying one binary field (the uderlying array)
might modify another binary field unintentionnaly (because the user
misunderstood how ByteBuffers worked and assumed the buffer's array was all
his). Therefore I don't think ByteBuffers would be the best choice if Thrift
was started from scratch today.
But we're not talking about rewriting Thrift...
bq. I think that the cost of giving a backwards compatible interface to
collections is very high, and that's why I'm counseling that we don't do it.
On the performance benefit, the re-use of underlying buffers (byte[]) is only
done in selected transports, for other transports (TSocket, THttpClient), the
use of ByteBuffer induces a performance penalty since not only do we have to
allocate a new byte[] but also a ByteBuffer that wraps that array.
> Container types containing binary data are parameterized with ByteBuffer in
> the generated Java code
> ---------------------------------------------------------------------------------------------------
>
> Key: THRIFT-1035
> URL: https://issues.apache.org/jira/browse/THRIFT-1035
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler, Java - Library
> Affects Versions: 0.4, 0.5, 0.6, 0.7
> Environment: All
> Reporter: Mathias Herberts
> Attachments: THRIFT-1035-2.patch, THRIFT-1035.patch
>
>
> Since THRIFT-830, binary fields are internally handled using ByteBuffer.
> Release 0.4.0 was the first to expose the ByteBuffer to the outside world
> (replacing previous methods returning/accepting byte[]).
> THRIFT-882 lead to the methods accepting/returning byte[] being available
> again, as it was deemed more reasonable not to expose the ByteBuffer too much
> as their use could be cumbersome. This lead to 0.5.0 being backward
> compatible with 0.3.0 on the binary fields front.
> During that time, nobody noticed that container types that contained binary
> data had their generated Java code changed to collections parameterized with
> ByteBuffer instead of byte[].
> list<binary> -> List<ByteBuffer>
> set<binary> -> Set<ByteBuffer>
> map<binary,...> -> Map<ByteBuffer,...>
> map<...,binary> -> Map<...,ByteBuffer>
> This introduces confusion in the API and still exposes ByteBuffer when
> discussion on THRIFT-882 concluded this should be avoided.
> We need to provide a way to offer the original parameterization with byte[]
> as this will simplify working with that type of collection and thus will
> increase the odds of Thrift's adoption.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
