[ https://issues.apache.org/jira/browse/HBASE-5625?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13260677#comment-13260677 ]
jirapos...@reviews.apache.org commented on HBASE-5625: ------------------------------------------------------ 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? bq. bq. Tudor Scurtu wrote: bq. Added overload. Added exception comment for when there is insufficient space remaining in the buffer. So, the way this works, we just allocate N and hope that stuff fits inside N? If it doesn't we throw an exception? There is no correlation between data that comes across and the N allocation? 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? bq. bq. Tudor Scurtu wrote: bq. Added exception comment for when there is insufficient space remaining in the buffer. Is that what you meant? I am not understanding how the allocation works. It seems arbitrary unrelated to the actual result size that comes over from the server. Is that so? If so, it seems unfriendly throwing an exception when allocated size and what is returned from the server do not match. 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? bq. bq. Tudor Scurtu wrote: bq. You need to be able to pass your own buffer when you have to compose multiple values. bq. 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. Sounds good Tudor. 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. bq. bq. Tudor Scurtu wrote: bq. This was copied from original. Should I remove both occurences? Sorry. I did not notice it was problem on original. If you can fix it, that'd be sweet. 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 !? bq. bq. Tudor Scurtu wrote: bq. They are not complementary. bq. containsColumn = value exists bq. containsEmptyColumn = value exists & is empty byte array bq. containsNonEmptyColumn = value exists & is not empty byte array bq. The value could be missing, in which case all methods would return false. OK. If not clear from comments, please add your notes above. Will help those that come after. 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. bq. bq. Tudor Scurtu wrote: bq. This overloads an original method. A refactoring here would be a major non-backwards compatible change. Ok. Thanks for pointing this out. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4607/#review6623 ----------------------------------------------------------- On 2012-04-04 17:08:03, 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-04 17:08:03) 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/TestKeyValue.java fae6902 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