[
https://issues.apache.org/jira/browse/HBASE-5625?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13246480#comment-13246480
]
[email protected] commented on HBASE-5625:
------------------------------------------------------
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > Looks good. Some comments below.
Thanks! Please read the changes below.
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 531
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line531>
bq. >
bq. > How do we know this buffer is big enough? Maybe should add an
override that takes an offset into the buffer?
Added overload. Added exception comment for when there is insufficient space
remaining in the buffer.
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 1443
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line1443>
bq. >
bq. > Is this an override?
Yes. Modified the original method to call the new one with default parameters.
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 1448
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line1448>
bq. >
bq. > 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...)
Moved.
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 2001
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line2001>
bq. >
bq. > How do I know the buffer is big enough? Should there be an override
that takes an offset?
Added buffer offset parameter. We know exactly how many bytes we have to write
and how much free space we have in the buffer. An 'IllegalArgumentException' is
thrown when there isn't enough free space. Is that what you were asking?
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 2002
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line2002>
bq. >
bq. > 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.
Hope I fixed this.
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/client/Result.java, line 251
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97955#file97955line251>
bq. >
bq. > Please follow convention that the rest of the file has.
Hope I fixed this.
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/client/Result.java, line 255
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97955#file97955line255>
bq. >
bq. > ditto
Hope I fixed this.
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/client/Result.java, line 478
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97955#file97955line478>
bq. >
bq. > Rename hasColumn or isColumn.
This overloads an original method. A refactoring here would be a major
non-backwards compatible change.
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/client/Result.java, line 398
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97955#file97955line398>
bq. >
bq. > Rename hasContent or isNotEmpty or isNotEmptyColumn
This method was named to mirror 'containsColumn()'. See below.
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/client/Result.java, line 431
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97955#file97955line431>
bq. >
bq. > Why we have both isNonEmptyColumn and isEmptyColumn? Why not just
one and then check return with a !?
They are not complementary.
containsColumn = value exists
containsEmptyColumn = value exists & is empty byte array
containsNonEmptyColumn = value exists & is not empty byte array
The value could be missing, in which case all methods would return false.
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/test/java/org/apache/hadoop/hbase/client/TestResult.java, line 117
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97956#file97956line117>
bq. >
bq. > 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.
Refactored as 'doReadBenchmark()'; called from 'main()';
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 2056
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line2056>
bq. >
bq. > 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?
Added checks in 'TestKeyValue.testFirstLastOnRow()' for the new
'KeyValue.createFirstOnRow()' methods.
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 616
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line616>
bq. >
bq. > How do I know the buffer is big enough?
Added exception comment for when there is insufficient space remaining in the
buffer. Is that what you meant?
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/client/Result.java, line 297
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97955#file97955line297>
bq. >
bq. > This comment should be on the @return javadoc rather than here.
This was copied from original. Should I remove both occurences?
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/test/java/org/apache/hadoop/hbase/client/TestResult.java, line 178
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97956#file97956line178>
bq. >
bq. > 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.
Moved 'loadValue()' checks to their own test methods. Also took the liberty to
move 'getColumn()' checks in the same manner in order to keep consistency.
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 1138
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97954#file97954line1138>
bq. >
bq. > Why not create a ByteBuffer? or called ByteBuffer wrap? Why not
call it toByteBuffer?
You need to be able to pass your own buffer when you have to compose multiple
values.
I added a new method that uses 'ByteBuffer.wrap()'. I feel that this method
should contain the word 'value' in its name so as not to create the impression
that it is writing the entire underlying 'KeyValue' structure to the buffer, so
I called it 'getValueAsByteBuffer()'. Please comment if it is inadequate.
bq. On 2012-04-02 17:34:38, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/client/Result.java, line 87
bq. > <https://reviews.apache.org/r/4607/diff/1/?file=97955#file97955line87>
bq. >
bq. > This should be lazily instantiated
Fixed.
- Tudor
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4607/#review6623
-----------------------------------------------------------
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, 5625v6.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
