[
https://issues.apache.org/jira/browse/HBASE-5625?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13244364#comment-13244364
]
[email protected] commented on HBASE-5625:
------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4607/#review6623
-----------------------------------------------------------
Looks good. Some comments below.
src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14311>
How do we know this buffer is big enough? Maybe should add an override
that takes an offset into the buffer?
src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14312>
I like this refactoring.
src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14313>
How do I know the buffer is big enough?
src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14315>
Why not create a ByteBuffer? or called ByteBuffer wrap? Why not call it
toByteBuffer?
src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14318>
Is this an override?
src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14317>
Why pull out all these values here? Maybe we'll fail the first test (q1 is
not used by the first test and these extractions cost.. they create byte
arrays...)
src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14319>
How do I know the buffer is big enough? Should there be an override that
takes an offset?
src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14320>
Please follow the formatting that is present in the rest of the file.
Notice placement of exceptions and params. Do not add your own style. Thanks.
src/main/java/org/apache/hadoop/hbase/KeyValue.java
<https://reviews.apache.org/r/4607/#comment14321>
This is a critical base class. I'm nervous when it gets refactored. You
have tests for each of your changes? And all the old KV tests pass?
src/main/java/org/apache/hadoop/hbase/client/Result.java
<https://reviews.apache.org/r/4607/#comment14322>
This should be lazily instantiated
src/main/java/org/apache/hadoop/hbase/client/Result.java
<https://reviews.apache.org/r/4607/#comment14323>
Please follow convention that the rest of the file has.
src/main/java/org/apache/hadoop/hbase/client/Result.java
<https://reviews.apache.org/r/4607/#comment14324>
ditto
src/main/java/org/apache/hadoop/hbase/client/Result.java
<https://reviews.apache.org/r/4607/#comment14325>
This comment should be on the @return javadoc rather than here.
src/main/java/org/apache/hadoop/hbase/client/Result.java
<https://reviews.apache.org/r/4607/#comment14326>
Rename hasContent or isNotEmpty or isNotEmptyColumn
src/main/java/org/apache/hadoop/hbase/client/Result.java
<https://reviews.apache.org/r/4607/#comment14327>
Why we have both isNonEmptyColumn and isEmptyColumn? Why not just one and
then check return with a !?
src/main/java/org/apache/hadoop/hbase/client/Result.java
<https://reviews.apache.org/r/4607/#comment14328>
Rename hasColumn or isColumn.
src/test/java/org/apache/hadoop/hbase/client/TestResult.java
<https://reviews.apache.org/r/4607/#comment14310>
Or leave it and change the name of the test so it doesn't have the test
prefix. Add a main and have it call this. I think the method useful. Might
as well keep it.
src/test/java/org/apache/hadoop/hbase/client/TestResult.java
<https://reviews.apache.org/r/4607/#comment14329>
I think there needs to be tests for the new KV functionality because KV is
a fundamental type and we can't mess it up.
- Michael
On 2012-04-02 14:22:48, Tudor Scurtu wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4607/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-04-02 14:22:48)
bq.
bq.
bq. Review request for hbase.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. When calling Result.getValue(), an extra dummy KeyValue and its associated
underlying byte array are allocated, as well as a persistent buffer that will
contain the returned value.
bq.
bq. These can be avoided by reusing a static array for the dummy object and by
passing a ByteBuffer object as a value destination buffer to the read method.
bq.
bq.
bq. This addresses bug HBASE-5625.
bq. https://issues.apache.org/jira/browse/HBASE-5625
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. src/main/java/org/apache/hadoop/hbase/KeyValue.java 243d76f
bq. src/main/java/org/apache/hadoop/hbase/client/Result.java df0b3ef
bq. src/test/java/org/apache/hadoop/hbase/client/TestResult.java f9e29c2
bq.
bq. Diff: https://reviews.apache.org/r/4607/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Added value check to TestResult#testBasic and TestResult.testMultiVersion.
bq.
bq.
bq. Thanks,
bq.
bq. Tudor
bq.
bq.
> Avoid byte buffer allocations when reading a value from a Result object
> -----------------------------------------------------------------------
>
> Key: HBASE-5625
> URL: https://issues.apache.org/jira/browse/HBASE-5625
> Project: HBase
> Issue Type: Improvement
> Components: client
> Affects Versions: 0.92.1
> Reporter: Tudor Scurtu
> Assignee: Tudor Scurtu
> Labels: patch
> Fix For: 0.96.0
>
> Attachments: 5625.txt, 5625v2.txt, 5625v3.txt, 5625v4.txt, 5625v5.txt
>
>
> When calling Result.getValue(), an extra dummy KeyValue and its associated
> underlying byte array are allocated, as well as a persistent buffer that will
> contain the returned value.
> These can be avoided by reusing a static array for the dummy object and by
> passing a ByteBuffer object as a value destination buffer to the read method.
> The current functionality is maintained, and we have added a separate method
> call stack that employs the described changes. I will provide more details
> with the patch.
> Running tests with a profiler, the reduction of read time seems to be of up
> to 40%.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
