[
https://issues.apache.org/jira/browse/HBASE-8782?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13691433#comment-13691433
]
Lars George commented on HBASE-8782:
------------------------------------
Hi [~madani], I looked at the patch and have a few questions:
- Performance
My biggest concern is that you are invoking a full copy by the looks using
Bytes.getBytes(), which in turn calls readBytes() doing the copying. It is part
of the critical path and will mean a lot of churn since it is called for every
operation. If it is really needed, at the least we need to add a local (size
bound) cache that keeps the most common ones for lookup instead of doing a
copy-read on every operation. You can do that by adding a new
byteBufferToBytes() for example, that check the cache first, and only on misses
does the getBytes() call and then stores the result.
- Fix Issue
Having said the above, I am not sure why the patch actually is fixing your
problem. Looking at the original ByteBuffer.array(), which returns the internal
array, or using Bytes.readBytes(), which internally iterates over
ByteBuffer.get() calls, returning the same array byte by byte.
Why do you see a difference there? How did you come to the conclusion that this
was caused by the framed transport option of Thrift?
More importantly, can we create a test for this? We have some skilled
Thrift'lers here in HBase land, so we could ask for help on staging that from
the Thrift side?
- Nits
Please check the imports while you are at it, there seems to be unused imports
(Delete here). Please kindly clean those little things up too.
Overall, I am concerned about the patch as it is. It will not change the
outcome of the test suite, but seems to cause a performance regressions.
> Thrift2 can not parse values when using framed transport
> --------------------------------------------------------
>
> Key: HBASE-8782
> URL: https://issues.apache.org/jira/browse/HBASE-8782
> Project: HBase
> Issue Type: Bug
> Components: Thrift
> Affects Versions: 0.95.1
> Reporter: Hamed Madani
> Attachments: HBASE_8782.patch
>
>
> ThriftHBaseServiceHandler.java use .array() on table names , and values
> (family , qualifier in checkandDelete , etc) which resulted in incorrect
> values with framed transport. Replacing .array() with getBytes() fixed this
> problem. I've attached the patch
> EDIT: updated the patch to cover checkAndPut(), checkAndDelete()
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
