HBase
  1. HBase
  2. HBASE-5521

Move compression/decompression to an encoder specific encoding context

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.95.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      As part of working on HBASE-5313, we want to add a new columnar encoder/decoder. It makes sense to move compression to be part of encoder/decoder:
      1) a scanner for a columnar encoded block can do lazy decompression to a specific part of a key value object
      2) avoid an extra bytes copy from encoder to hblock-writer.

      If there is no encoder specified for a writer, the HBlock.Writer will use a default compression-context to do something very similar to today's code.

        Activity

        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4182/
        -----------------------------------------------------------

        Review request for hbase, Dhruba Borthakur and Scott Chen.

        Summary
        -------

        Move compression/decompression to an encoder specific encoding context

        This addresses bug HBASE-5521.
        https://issues.apache.org/jira/browse/HBASE-5521

        Diffs


        /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java 1297201
        /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java 1297201
        /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java 1297201
        /src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java 1296006
        /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java 1297201
        /src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java PRE-CREATION
        /src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java PRE-CREATION
        /src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java PRE-CREATION
        /src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java PRE-CREATION
        /src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java 1297201
        /src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java 1297201
        /src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java 1297201
        /src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java 1297201
        /src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java 1297201
        /src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java 1297201
        /src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java 1297201
        /src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java 1297201
        /src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java 1297201

        Diff: https://reviews.apache.org/r/4182/diff

        Testing
        -------

        Thanks,

        Yongqiang

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4182/ ----------------------------------------------------------- Review request for hbase, Dhruba Borthakur and Scott Chen. Summary ------- Move compression/decompression to an encoder specific encoding context This addresses bug HBASE-5521 . https://issues.apache.org/jira/browse/HBASE-5521 Diffs /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java 1297201 /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java 1297201 /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java 1297201 /src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java 1296006 /src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java 1297201 /src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java PRE-CREATION /src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java PRE-CREATION /src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java PRE-CREATION /src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java PRE-CREATION /src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java 1297201 /src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java 1297201 /src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java 1297201 /src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java 1297201 /src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java 1297201 /src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java 1297201 /src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java 1297201 /src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java 1297201 /src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java 1297201 Diff: https://reviews.apache.org/r/4182/diff Testing ------- Thanks, Yongqiang
        Hide
        He Yongqiang added a comment -
        Show
        He Yongqiang added a comment - moved the review to https://reviews.facebook.net/D2097
        Hide
        Phabricator added a comment -

        heyongqiang requested code review of "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".
        Reviewers: JIRA

        https://issues.apache.org/jira/browse/HBASE-5521

        As part of working on HBASE-5313, we want to add a new columnar encoder/decoder. It makes sense to move compression to be part of encoder/decoder:
        1) a scanner for a columnar encoded block can do lazy decompression to a specific part of a key value object
        2) avoid an extra bytes copy from encoder to hblock-writer.

        If there is no encoder specified for a writer, the HBlock.Writer will use a default compression-context to do something very similar to today's code.

        TEST PLAN
        EMPTY

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        AFFECTED FILES
        src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java

        MANAGE HERALD DIFFERENTIAL RULES
        https://reviews.facebook.net/herald/view/differential/

        WHY DID I GET THIS EMAIL?
        https://reviews.facebook.net/herald/transcript/4539/

        Tip: use the X-Herald-Rules header to filter Herald messages in your client.

        Show
        Phabricator added a comment - heyongqiang requested code review of " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Reviewers: JIRA https://issues.apache.org/jira/browse/HBASE-5521 As part of working on HBASE-5313 , we want to add a new columnar encoder/decoder. It makes sense to move compression to be part of encoder/decoder: 1) a scanner for a columnar encoded block can do lazy decompression to a specific part of a key value object 2) avoid an extra bytes copy from encoder to hblock-writer. If there is no encoder specified for a writer, the HBlock.Writer will use a default compression-context to do something very similar to today's code. TEST PLAN EMPTY REVISION DETAIL https://reviews.facebook.net/D2097 AFFECTED FILES src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/4539/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        Still going over the new Context interfaces.

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:51 I don't find where this method is used.
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:690 There is nothing to be done between preparation and post processing ?
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:693 nonDataBlockEncodingCtx should be used here, I assume.

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Still going over the new Context interfaces. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:51 I don't find where this method is used. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:690 There is nothing to be done between preparation and post processing ? src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:693 nonDataBlockEncodingCtx should be used here, I assume. REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:811 Should we perform similar handling for nonDataBlockEncodingCtx ?
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:810 dataBlockEncodingCtx should be set to null so that release() is re-entrant.

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:811 Should we perform similar handling for nonDataBlockEncodingCtx ? src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:810 dataBlockEncodingCtx should be set to null so that release() is re-entrant. REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Phabricator added a comment -

        mbautin has requested changes to the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        Yongqiang: thanks for taking on this refactoring project. I have added some comments inline.

        What is "columnar encoded block format", by the way? Also, it is not totally clear to me why we would need to unite compression and encoding steps in order to support "columnar encoded" block format.

        This requires a test plan and new unit tests. Please add test cases for "encoding context" implementations.

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:58 Why do we need two separate compressKeyValues functions? What is the difference in use cases? Overloaded methods make the interface confusing.

        Add postEncoding javadoc.
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:179-183 Javadoc please.
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:53 Why do we need to take encoding as an explicit parameter? Can we figure it out from dataBlockEncoder?
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java:323-327 Is this actually what you wanted to check? This code as written will accept any subclass of HFileBlockDefaultEncodingContext. If you want to only accept an instance of the HFileBlockDefaultEncodingContext class specifically, you need

        encodingCxt.getClass() == HFileBlockDefaultEncodingContext.class

        However, why do we need to enforce such a constraint? Would not an arbitrary implementation of HFileBlockEncodingContext be acceptable here?

        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1396 Code style: add spaces after //. Capitalize the first word in a sentence. In short: be consistent with the existing style of the surrounding code.
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1327-1328 Why did you remove the comment about the peek-into-next-block-header optimization?

        here we already read -> here we have already read

        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:194-195 Why do we need to instantiate the "encoding context" for every encoding operation?
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1253 Why do we need to have a separate "default encoding context" instance here?

        Can you make the default context a singleton (or a per-compression-type singleton) and use the relevant unique instance instead of this field?
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1357 Code style: space after "if".

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        BRANCH
        svn

        Show
        Phabricator added a comment - mbautin has requested changes to the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Yongqiang: thanks for taking on this refactoring project. I have added some comments inline. What is "columnar encoded block format", by the way? Also, it is not totally clear to me why we would need to unite compression and encoding steps in order to support "columnar encoded" block format. This requires a test plan and new unit tests. Please add test cases for "encoding context" implementations. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:58 Why do we need two separate compressKeyValues functions? What is the difference in use cases? Overloaded methods make the interface confusing. Add postEncoding javadoc. src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:179-183 Javadoc please. src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:53 Why do we need to take encoding as an explicit parameter? Can we figure it out from dataBlockEncoder? src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java:323-327 Is this actually what you wanted to check? This code as written will accept any subclass of HFileBlockDefaultEncodingContext. If you want to only accept an instance of the HFileBlockDefaultEncodingContext class specifically, you need encodingCxt.getClass() == HFileBlockDefaultEncodingContext.class However, why do we need to enforce such a constraint? Would not an arbitrary implementation of HFileBlockEncodingContext be acceptable here? src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1396 Code style: add spaces after //. Capitalize the first word in a sentence. In short: be consistent with the existing style of the surrounding code. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1327-1328 Why did you remove the comment about the peek-into-next-block-header optimization? here we already read -> here we have already read src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:194-195 Why do we need to instantiate the "encoding context" for every encoding operation? src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1253 Why do we need to have a separate "default encoding context" instance here? Can you make the default context a singleton (or a per-compression-type singleton) and use the relevant unique instance instead of this field? src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1357 Code style: space after "if". REVISION DETAIL https://reviews.facebook.net/D2097 BRANCH svn
        Hide
        Phabricator added a comment -

        heyongqiang has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java:323-327 good suggestion. i will change to use class for checking.

        Yes, we need to enforce that contains right now. Basically we want each encoder to use its own context object which is returned by newDataBlockEncodingContext()

        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:58 will remove the original method, and change all testcases to use the new api.

        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:53 are you suggesting adding a new field Encoding to each encoder?
        i also think it is a good thing to do, but not sure the reason of why the current code not doing that.
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:693 Good catch! this is a bug.
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:811 good catch here. will assign to null.
        Initially i am doing that is because they are all defined as 'final'. will remove the final.
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1253 default is used for nondata block. I can not make them singleton as they need to maintain some reusable objects internally. By reusable objects, it is basically some buffers allocated once and can be reused across multiple operations.

        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1327-1328 added back
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:194-195 good catch. will add a class field to reuse it

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        BRANCH
        svn

        Show
        Phabricator added a comment - heyongqiang has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java:323-327 good suggestion. i will change to use class for checking. Yes, we need to enforce that contains right now. Basically we want each encoder to use its own context object which is returned by newDataBlockEncodingContext() src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:58 will remove the original method, and change all testcases to use the new api. src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:53 are you suggesting adding a new field Encoding to each encoder? i also think it is a good thing to do, but not sure the reason of why the current code not doing that. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:693 Good catch! this is a bug. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:811 good catch here. will assign to null. Initially i am doing that is because they are all defined as 'final'. will remove the final. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1253 default is used for nondata block. I can not make them singleton as they need to maintain some reusable objects internally. By reusable objects, it is basically some buffers allocated once and can be reused across multiple operations. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1327-1328 added back src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:194-195 good catch. will add a class field to reuse it REVISION DETAIL https://reviews.facebook.net/D2097 BRANCH svn
        Hide
        Phabricator added a comment -

        heyongqiang updated the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".
        Reviewers: JIRA, dhruba, tedyu, sc, mbautin

        address Ted and mbautin's comments.

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        AFFECTED FILES
        src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java

        Show
        Phabricator added a comment - heyongqiang updated the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Reviewers: JIRA, dhruba, tedyu, sc, mbautin address Ted and mbautin's comments. REVISION DETAIL https://reviews.facebook.net/D2097 AFFECTED FILES src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java
        Hide
        Phabricator added a comment -

        mbautin has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        Yongqiang: thanks for addressing the comments. I will take another pass through the new version diff.

        Could you please use a consistent abbreviation for "context"? Either Ctx or Cxt, but not both.

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:53 HFileDataBlockEncoder is actually aware of on-disk and in-cache encoding. The on-disk encoding may be disabled when in-cache encoding is enabled, but not the other way. The current code apparently did not need to explicitly store encoding type in EncodedDataBlock (which is mostly used for testing and benchmarking), and I am wondering why you need to store it now.

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - mbautin has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Yongqiang: thanks for addressing the comments. I will take another pass through the new version diff. Could you please use a consistent abbreviation for "context"? Either Ctx or Cxt, but not both. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:53 HFileDataBlockEncoder is actually aware of on-disk and in-cache encoding. The on-disk encoding may be disabled when in-cache encoding is enabled, but not the other way. The current code apparently did not need to explicitly store encoding type in EncodedDataBlock (which is mostly used for testing and benchmarking), and I am wondering why you need to store it now. REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Phabricator added a comment -

        heyongqiang has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:53 I think here it is DataBlockEncoder (prefix, diff etc), and not HFileDataBlockEncoder

        From encoding, you can get encoder. but from encoder, can not get encoding.

        The class field encoding is not needed, will remove it.

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - heyongqiang has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:53 I think here it is DataBlockEncoder (prefix, diff etc), and not HFileDataBlockEncoder From encoding, you can get encoder. but from encoder, can not get encoding. The class field encoding is not needed, will remove it. REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Phabricator added a comment -

        heyongqiang updated the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".
        Reviewers: JIRA, dhruba, tedyu, sc, mbautin

        add mbautin's comments, and changed all Cxt to Ctx

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        AFFECTED FILES
        src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java

        Show
        Phabricator added a comment - heyongqiang updated the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Reviewers: JIRA, dhruba, tedyu, sc, mbautin add mbautin's comments, and changed all Cxt to Ctx REVISION DETAIL https://reviews.facebook.net/D2097 AFFECTED FILES src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java
        Hide
        Phabricator added a comment -

        heyongqiang updated the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".
        Reviewers: JIRA, dhruba, tedyu, sc, mbautin

        fix the test failure.

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        AFFECTED FILES
        src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java

        Show
        Phabricator added a comment - heyongqiang updated the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Reviewers: JIRA, dhruba, tedyu, sc, mbautin fix the test failure. REVISION DETAIL https://reviews.facebook.net/D2097 AFFECTED FILES src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12517375/HBASE-5521.D2097.4.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 12 new or modified tests.

        -1 javadoc. The javadoc tool appears to have generated -125 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 158 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1128//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1128//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1128//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12517375/HBASE-5521.D2097.4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -125 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 158 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1128//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1128//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1128//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:67 Should inCache be used here instead of NONE ?
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:235 Add a space between if and (
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:237 Add a space between if and (

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:67 Should inCache be used here instead of NONE ? src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:235 Add a space between if and ( src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:237 Add a space between if and ( REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Phabricator added a comment -

        mbautin has requested changes to the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        Yongqiang: thanks for the revised version. I have some more comments (inline). Also, could you please add some more unit tests specifically for the encoding/decoding context stuff?

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java:328 Rename this to internalEncodeKeyValues ("Encoding" -> "Encode").

        Also, this should probably be at least protected, because a public method called "internalFoo" is confusing.
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java:336 Please replace this with just

        if (encodingCtx.getClass() != HFileBlockDefaultEncodingContext.class)

        This works because class objects are unique in Java within the same class loader.

        http://stackoverflow.com/questions/3738919/does-java-guarantee-that-object-getclass-object-getclass
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java:348 What if encoding type is NONE? Would ENCODED_DATA still be valid here?
        src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java:36 -> internalEncodeKeyValues
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:28-37 Update the Javadoc to reflect the fact that encoder also deals with block compression and describe the relation between these two steps.
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java:109 wrong parameter name
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:50 Fill out the description for the encoding parameter
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:56 Get rid of this variable and use the constant directly
        src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java:346 -> internalEncodeKeyValues
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java:25 Readability:

        one reader's -> a reader's
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java:38 need to done -> need to be done
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java:42-44 It is not clear what parameter is input and what is output. Please fill in the descriptions.
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java:39 Make this final
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:34 Not sure what is meant by "as a whole bytes buffer". Could you please re-word this?
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:59-60 Do these need to be package-private? Can they be made private?
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:26 the write's whole lifetime -> the writer's whole lifetime (I assume that is what you meant)
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:25 one writer's encoder -> a writer's encoder

        The "one writer's encoder" wording hints that an encoding context may potentially be shared with other writers. I am assuming that is not the case. If it is, could you please clarify the scope of sharing of an encoding context?
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:54 It is not clear what "prepare an encoding" actually means. Could you please provide more detail here?
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:67 any action that need -> any action that needs
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:68-69 NONE could also be considered a "valid compression algorithm" by some. Please remove the ambiguity.
        src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java:78 -> internalEncodeKeyValues
        src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java:332-333 Add descriptions. It looks like there is an assumption that the dest byte array has enough space allocated to store the decompressed block.
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:538 Add javadoc clarifying in what cases we use this context.
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:687 Moving compression into the encoding framework introduces the need for separate logic for compressing data blocks and all other blocks. Is it possible to reduce code duplication between compression for data and non-data blocks?
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:795 Code style: add a space after "if".
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1397 to it -> in it.

        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1421 Make this line less than 80 characters.
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java:94 create a encoder -> Create an encoder
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:44 Can this be made private and final?
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:171 The function used to return its result but now it does not. Please specify what is the function's output in the javadoc.
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:193 Add javadoc describing the input and the output.
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:235 Add space between if and (
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:237 Add space between if and (
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:249 Add space between if and (
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:251 Add space between if and (
        src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java:53 Where is the output now? Add javadoc.
        src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java:65 This seems to assume that the payload starts at the beginning of the buffer's backing array, and discards the buffer's arrayOffset() and limit(). Is that intended? Why pass a ByteBuffer then?

        Alternatively, should compressAfterEncoding take a starting offset and a length along with the byte array?
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java:100-102 Fix long line
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java:119-123 Remove commented-out code
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java:100 Does "Baos" stand for "Byte array output stream"? This function returns a ByteArrayInputStream.

        Please rename this function to remove the confusion. Also, please avoid using "Baos" in function names.

        Also please add javadoc.
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java:385 Please make this an assertion with a readable error message. No one will notice "mismatch.." in test output or understand what it means.
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java:447 Add space between if and (

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        BRANCH
        svn

        Show
        Phabricator added a comment - mbautin has requested changes to the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Yongqiang: thanks for the revised version. I have some more comments (inline). Also, could you please add some more unit tests specifically for the encoding/decoding context stuff? INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java:328 Rename this to internalEncodeKeyValues ("Encoding" -> "Encode"). Also, this should probably be at least protected, because a public method called "internalFoo" is confusing. src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java:336 Please replace this with just if (encodingCtx.getClass() != HFileBlockDefaultEncodingContext.class) This works because class objects are unique in Java within the same class loader. http://stackoverflow.com/questions/3738919/does-java-guarantee-that-object-getclass-object-getclass src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java:348 What if encoding type is NONE? Would ENCODED_DATA still be valid here? src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java:36 -> internalEncodeKeyValues src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:28-37 Update the Javadoc to reflect the fact that encoder also deals with block compression and describe the relation between these two steps. src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java:109 wrong parameter name src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:50 Fill out the description for the encoding parameter src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:56 Get rid of this variable and use the constant directly src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java:346 -> internalEncodeKeyValues src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java:25 Readability: one reader's -> a reader's src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java:38 need to done -> need to be done src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java:42-44 It is not clear what parameter is input and what is output. Please fill in the descriptions. src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java:39 Make this final src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:34 Not sure what is meant by "as a whole bytes buffer". Could you please re-word this? src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:59-60 Do these need to be package-private? Can they be made private? src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:26 the write's whole lifetime -> the writer's whole lifetime (I assume that is what you meant) src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:25 one writer's encoder -> a writer's encoder The "one writer's encoder" wording hints that an encoding context may potentially be shared with other writers. I am assuming that is not the case. If it is, could you please clarify the scope of sharing of an encoding context? src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:54 It is not clear what "prepare an encoding" actually means. Could you please provide more detail here? src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:67 any action that need -> any action that needs src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:68-69 NONE could also be considered a "valid compression algorithm" by some. Please remove the ambiguity. src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java:78 -> internalEncodeKeyValues src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java:332-333 Add descriptions. It looks like there is an assumption that the dest byte array has enough space allocated to store the decompressed block. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:538 Add javadoc clarifying in what cases we use this context. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:687 Moving compression into the encoding framework introduces the need for separate logic for compressing data blocks and all other blocks. Is it possible to reduce code duplication between compression for data and non-data blocks? src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:795 Code style: add a space after "if". src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1397 to it -> in it. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1421 Make this line less than 80 characters. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java:94 create a encoder -> Create an encoder src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:44 Can this be made private and final? src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:171 The function used to return its result but now it does not. Please specify what is the function's output in the javadoc. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:193 Add javadoc describing the input and the output. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:235 Add space between if and ( src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:237 Add space between if and ( src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:249 Add space between if and ( src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:251 Add space between if and ( src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java:53 Where is the output now? Add javadoc. src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java:65 This seems to assume that the payload starts at the beginning of the buffer's backing array, and discards the buffer's arrayOffset() and limit(). Is that intended? Why pass a ByteBuffer then? Alternatively, should compressAfterEncoding take a starting offset and a length along with the byte array? src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java:100-102 Fix long line src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java:119-123 Remove commented-out code src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java:100 Does "Baos" stand for "Byte array output stream"? This function returns a ByteArrayInputStream. Please rename this function to remove the confusion. Also, please avoid using "Baos" in function names. Also please add javadoc. src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java:385 Please make this an assertion with a readable error message. No one will notice "mismatch.." in test output or understand what it means. src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java:447 Add space between if and ( REVISION DETAIL https://reviews.facebook.net/D2097 BRANCH svn
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:56 This parameter is not in method signature any more.

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        BRANCH
        svn

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:56 This parameter is not in method signature any more. REVISION DETAIL https://reviews.facebook.net/D2097 BRANCH svn
        Hide
        Phabricator added a comment -

        heyongqiang updated the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".
        Reviewers: JIRA, dhruba, tedyu, sc, mbautin

        address mbautin and Ted's coents.

        @mbautin, i do not think we need to add a testcase for the new default contclasses as th are recovered by exising codebase pretty well.

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        AFFECTED FILES
        src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java

        Show
        Phabricator added a comment - heyongqiang updated the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Reviewers: JIRA, dhruba, tedyu, sc, mbautin address mbautin and Ted's coents. @mbautin, i do not think we need to add a testcase for the new default contclasses as th are recovered by exising codebase pretty well. REVISION DETAIL https://reviews.facebook.net/D2097 AFFECTED FILES src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java
        Hide
        Phabricator added a comment -

        heyongqiang has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        @mbautin, i do not think we need to add a testcase for the new default context classes, as they are covered by existing testcase/codes pretty well. i will add more test cases after adding the new encoder type.

        INLINE COMMENTS
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java:385 good catch. i added that just for my debugging

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - heyongqiang has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". @mbautin, i do not think we need to add a testcase for the new default context classes, as they are covered by existing testcase/codes pretty well. i will add more test cases after adding the new encoder type. INLINE COMMENTS src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java:385 good catch. i added that just for my debugging REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12517499/HBASE-5521.D2097.5.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 12 new or modified tests.

        -1 javadoc. The javadoc tool appears to have generated -127 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 158 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.regionserver.TestAtomicOperation
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestImportTsv

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1129//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1129//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1129//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12517499/HBASE-5521.D2097.5.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -127 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 158 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.regionserver.TestAtomicOperation org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestImportTsv Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1129//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1129//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1129//console This message is automatically generated.
        Hide
        He Yongqiang added a comment -

        Tests passed in my local test, not sure why failed on Jenkins.

        Show
        He Yongqiang added a comment - Tests passed in my local test, not sure why failed on Jenkins.
        Hide
        Phabricator added a comment -

        heyongqiang updated the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".
        Reviewers: JIRA, dhruba, tedyu, sc, mbautin

        rebase with trunk

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        AFFECTED FILES
        src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockCompatibility.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java

        Show
        Phabricator added a comment - heyongqiang updated the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Reviewers: JIRA, dhruba, tedyu, sc, mbautin rebase with trunk REVISION DETAIL https://reviews.facebook.net/D2097 AFFECTED FILES src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockCompatibility.java src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12518539/HBASE-5521.D2097.6.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 15 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 166 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.io.hfile.TestHFileWriterV2
        org.apache.hadoop.hbase.coprocessor.TestProcessRowEndpoint
        org.apache.hadoop.hbase.io.hfile.TestHFile
        org.apache.hadoop.hbase.io.hfile.TestReseekTo

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1194//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1194//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1194//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12518539/HBASE-5521.D2097.6.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 166 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.io.hfile.TestHFileWriterV2 org.apache.hadoop.hbase.coprocessor.TestProcessRowEndpoint org.apache.hadoop.hbase.io.hfile.TestHFile org.apache.hadoop.hbase.io.hfile.TestReseekTo Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1194//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1194//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1194//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        mbautin has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        Yongqiang: we now have a linter available in HBase trunk. Could you please run "arc lint", resolve lint warnings, and resubmit the diff with "arc diff --preview"?

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - mbautin has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Yongqiang: we now have a linter available in HBase trunk. Could you please run "arc lint", resolve lint warnings, and resubmit the diff with "arc diff --preview"? REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Phabricator added a comment -

        heyongqiang updated the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".
        Reviewers: JIRA, dhruba, tedyu, sc, mbautin

        fix tests failures

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        AFFECTED FILES
        src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockCompatibility.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java

        Show
        Phabricator added a comment - heyongqiang updated the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Reviewers: JIRA, dhruba, tedyu, sc, mbautin fix tests failures REVISION DETAIL https://reviews.facebook.net/D2097 AFFECTED FILES src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockCompatibility.java src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java
        Hide
        Mikhail Bautin added a comment -

        +1

        I have sucessfully re-run tests that failed on Jenkins locally:

        Running org.apache.hadoop.hbase.io.hfile.TestHFileWriterV2
        Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.904 sec
        Running org.apache.hadoop.hbase.io.hfile.TestReseekTo
        Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.846 sec
        Running org.apache.hadoop.hbase.io.hfile.TestHFile
        Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.11 sec
        Running org.apache.hadoop.hbase.coprocessor.TestProcessRowEndpoint
        Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 20.273 sec

        Results :

        Tests run: 10, Failures: 0, Errors: 0, Skipped: 0

        @Committers: this patch has been out for a while, and Yongqiang is addressing final minor comments. Would anyone else +1 this?

        Show
        Mikhail Bautin added a comment - +1 I have sucessfully re-run tests that failed on Jenkins locally: Running org.apache.hadoop.hbase.io.hfile.TestHFileWriterV2 Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.904 sec Running org.apache.hadoop.hbase.io.hfile.TestReseekTo Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.846 sec Running org.apache.hadoop.hbase.io.hfile.TestHFile Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.11 sec Running org.apache.hadoop.hbase.coprocessor.TestProcessRowEndpoint Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 20.273 sec Results : Tests run: 10, Failures: 0, Errors: 0, Skipped: 0 @Committers: this patch has been out for a while, and Yongqiang is addressing final minor comments. Would anyone else +1 this?
        Hide
        Ted Yu added a comment -

        I have verified that failed tests pass now.

        There're a lot of whitespace warnings from lint:

        >>> Lint for src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java:
        
           Error  (TXT6) Trailing Whitespace
            This line contains trailing whitespace.
        

        Please give me a few moment in going over the patch one more time.

        Show
        Ted Yu added a comment - I have verified that failed tests pass now. There're a lot of whitespace warnings from lint: >>> Lint for src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java: Error (TXT6) Trailing Whitespace This line contains trailing whitespace. Please give me a few moment in going over the patch one more time.
        Hide
        Phabricator added a comment -

        mbautin has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        Yongqiang: here are some more comments, mostly very minor, but a couple of real ones, too.

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:38 It -> it
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:79 Remove unnecessary concatenation
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:644 Add an empty line here
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:829 Code style: add space after (int)
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:952-955 Could the close operation fail? If so, would it make sense to attempt the second close operation in a finally block?
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1160 Nice! +1 on making more fields final.
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1445-1446 Can these be made private?
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1729 Code style: space between if and (
        I will add a lint rule to catch this, too.
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1742-1745 nit: in multi-line comments like this, please capitalize the first word and end sentences with a full stop.
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java:94 [nit] create -> Create
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java:111 nit: specify what this returns, or remove @return
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:42-45 Do we have any subclasses of HFileDataBlockEncoderImpl, or are these fields accessed from other classes in the package? If latter, please change access to package-private. This is a more restrictive access level than protected, because classes from the same package can actually access protected members (I was surprised when I learned about this).
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:86 nit: use a // comment
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:263-276 Here is probably one of the few important comments in my current batch. There is code duplication between these two methods. Can we merge these two methods in the HFile encoder interface into one, and get rid of this code duplication in the implementation?
        src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java:41 Why does this have to be protected? This class is a singleton and we should not create it anywhere else. Instead, use NoOpDataBlockEncoder.INSTANCE.
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java:358 Is DataBlockEncoding.getAllEncoders() used anywhere after you have removed this line? If not, I think we should get rid of it since it is confusing to have a function called getAllEncoders() that does not actually return all encoders.
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockCompatibility.java:20 This file contains a lot of code duplicated from TestHFileBlock. Dhruba's intent, as I understood it, to have a "frozen" implementation without HBase-level checksums that tests would use to ensure compatibility before and after the checksum patch. Therefore, we should probably minimize changes to this file and emphasize that in comments explicitly and conspicuously.

        Please work with Dhruba to address the TestHFileBlockCompatibility issues.

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - mbautin has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Yongqiang: here are some more comments, mostly very minor, but a couple of real ones, too. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:38 It -> it src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:79 Remove unnecessary concatenation src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:644 Add an empty line here src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:829 Code style: add space after (int) src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:952-955 Could the close operation fail? If so, would it make sense to attempt the second close operation in a finally block? src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1160 Nice! +1 on making more fields final. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1445-1446 Can these be made private? src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1729 Code style: space between if and ( I will add a lint rule to catch this, too. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1742-1745 nit: in multi-line comments like this, please capitalize the first word and end sentences with a full stop. src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java:94 [nit] create -> Create src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java:111 nit: specify what this returns, or remove @return src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:42-45 Do we have any subclasses of HFileDataBlockEncoderImpl, or are these fields accessed from other classes in the package? If latter, please change access to package-private. This is a more restrictive access level than protected, because classes from the same package can actually access protected members (I was surprised when I learned about this). src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:86 nit: use a // comment src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:263-276 Here is probably one of the few important comments in my current batch. There is code duplication between these two methods. Can we merge these two methods in the HFile encoder interface into one, and get rid of this code duplication in the implementation? src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java:41 Why does this have to be protected? This class is a singleton and we should not create it anywhere else. Instead, use NoOpDataBlockEncoder.INSTANCE. src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java:358 Is DataBlockEncoding.getAllEncoders() used anywhere after you have removed this line? If not, I think we should get rid of it since it is confusing to have a function called getAllEncoders() that does not actually return all encoders. src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockCompatibility.java:20 This file contains a lot of code duplicated from TestHFileBlock. Dhruba's intent, as I understood it, to have a "frozen" implementation without HBase-level checksums that tests would use to ensure compatibility before and after the checksum patch. Therefore, we should probably minimize changes to this file and emphasize that in comments explicitly and conspicuously. Please work with Dhruba to address the TestHFileBlockCompatibility issues. REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12518742/HBASE-5521.D2097.7.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 15 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 165 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.util.TestHBaseFsck

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1211//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1211//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1211//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12518742/HBASE-5521.D2097.7.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 165 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.util.TestHBaseFsck Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1211//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1211//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1211//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        Good progress.

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java:330 Extra empty line.
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:55 Can 'contain returning results' be explained a little more ?
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1624 'to seek back' can be replaced with 'backward'
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:38 'compress' -> 'compresses'
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:60 Since compression is optional, should this method be named encodeKeyValuesFollowedByOptionalCompression
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:122 Would createDataBlockEncodingContext be a better name ?
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:127 'an encoder'
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:200 Are this javadoc and method name consistent with what line 212 returns ?
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:64 Please add javadoc for the parameters
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:133 Add headerBytes to @param
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:208 'decoding' -> 'compression'
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:156 Please add @Override for methods in the interface.

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Good progress. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java:330 Extra empty line. src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:55 Can 'contain returning results' be explained a little more ? src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:1624 'to seek back' can be replaced with 'backward' src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:38 'compress' -> 'compresses' src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:60 Since compression is optional, should this method be named encodeKeyValuesFollowedByOptionalCompression src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:122 Would createDataBlockEncodingContext be a better name ? src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:127 'an encoder' src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:200 Are this javadoc and method name consistent with what line 212 returns ? src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:64 Please add javadoc for the parameters src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:133 Add headerBytes to @param src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:208 'decoding' -> 'compression' src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:156 Please add @Override for methods in the interface. REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Phabricator added a comment -

        heyongqiang updated the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".
        Reviewers: JIRA, dhruba, tedyu, sc, mbautin

        address mbautin & tedyu's comments.

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        AFFECTED FILES
        src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockCompatibility.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java

        Show
        Phabricator added a comment - heyongqiang updated the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Reviewers: JIRA, dhruba, tedyu, sc, mbautin address mbautin & tedyu's comments. REVISION DETAIL https://reviews.facebook.net/D2097 AFFECTED FILES src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockCompatibility.java src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:202 How about naming this method encodeData() ?
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:106 Typo: 'to start'
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:109 I think this method should be in the interface.
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:34 When getOnDiskBytesWithHeader() is called, encoding is done already, right ?
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:131 I think this method should be in the interface.
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:177 Is this method called anywhere ?
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:56 Does 'as well as' mean 'or' here ?
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:61 I still think this method should be renamed because compression is optional.

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:202 How about naming this method encodeData() ? src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:106 Typo: 'to start' src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:109 I think this method should be in the interface. src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:34 When getOnDiskBytesWithHeader() is called, encoding is done already, right ? src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:131 I think this method should be in the interface. src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:177 Is this method called anywhere ? src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:56 Does 'as well as' mean 'or' here ? src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:61 I still think this method should be renamed because compression is optional. REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12518757/HBASE-5521.D2097.8.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 15 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 165 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestImportTsv

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1214//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1214//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1214//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12518757/HBASE-5521.D2097.8.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 165 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestImportTsv Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1214//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1214//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1214//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        heyongqiang has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:61 The interface is there before this change.
        I agree it need a better name. But i think that should be done in a different context.
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:109 this interface is removed from the interface as to address your previous comments. Methods defined in an interface need to be clear and need to be as fewer as possible. Once it is added, it is not easy to take back.
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:34 Yes. It returns encoded uncompressed bytes.
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:202 ok. i will rename it to encodeData()
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:106 will fix
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:56 nope. it's 'as well as'...will put a few more words here

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - heyongqiang has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:61 The interface is there before this change. I agree it need a better name. But i think that should be done in a different context. src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:109 this interface is removed from the interface as to address your previous comments. Methods defined in an interface need to be clear and need to be as fewer as possible. Once it is added, it is not easy to take back. src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:34 Yes. It returns encoded uncompressed bytes. src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java:202 ok. i will rename it to encodeData() src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:106 will fix src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:56 nope. it's 'as well as'...will put a few more words here REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Phabricator added a comment -

        heyongqiang updated the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".
        Reviewers: JIRA, dhruba, tedyu, sc, mbautin

        address ted's comments:
        fixed a typo, rename a function name etc

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        AFFECTED FILES
        src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockCompatibility.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java

        Show
        Phabricator added a comment - heyongqiang updated the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Reviewers: JIRA, dhruba, tedyu, sc, mbautin address ted's comments: fixed a typo, rename a function name etc REVISION DETAIL https://reviews.facebook.net/D2097 AFFECTED FILES src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockCompatibility.java src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12518781/HBASE-5521.D2097.9.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 15 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 164 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat
        org.apache.hadoop.hbase.master.TestSplitLogManager

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1217//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1217//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1217//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12518781/HBASE-5521.D2097.9.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 164 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat org.apache.hadoop.hbase.master.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1217//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1217//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1217//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        Looks good.
        I have only one comment outstanding w.r.t. compressKeyValues()

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:61 I looked at PrefixKeyDeltaEncoder.compressKeyValues(DataOutputStream ...)

        It only does encoding (and appending memstoreTs).

        I think we should take this opportunity to make interface method name(s) clearer.
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:109 Pardon me if my comments have been inconsistent.

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Looks good. I have only one comment outstanding w.r.t. compressKeyValues() INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:61 I looked at PrefixKeyDeltaEncoder.compressKeyValues(DataOutputStream ...) It only does encoding (and appending memstoreTs). I think we should take this opportunity to make interface method name(s) clearer. src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java:109 Pardon me if my comments have been inconsistent. REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:34 Can we make this javadoc clearer ?
        Something like:
        @return encoded bytes which are ready ...

        This would leave no confusion as to when encoding is done.
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:39 Similar to comment above.
        When 'after encoding' is placed at the end of the sentence, I am not sure interpretation of where encoding is done would be uniform.

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:34 Can we make this javadoc clearer ? Something like: @return encoded bytes which are ready ... This would leave no confusion as to when encoding is done. src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:39 Similar to comment above. When 'after encoding' is placed at the end of the sentence, I am not sure interpretation of where encoding is done would be uniform. REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Phabricator added a comment -

        heyongqiang has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        will address the java doc etc comments.

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:61 It will also do the compression.
        The internalEncodeKeyValues() only does the encoding. but compressKeyValues will do encoding (by calling internalEncode...) and compress in postEncoding().

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - heyongqiang has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". will address the java doc etc comments. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java:61 It will also do the compression. The internalEncodeKeyValues() only does the encoding. but compressKeyValues will do encoding (by calling internalEncode...) and compress in postEncoding(). REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Phabricator added a comment -

        heyongqiang updated the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".
        Reviewers: JIRA, dhruba, tedyu, sc, mbautin

        address javadoc comments...

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        AFFECTED FILES
        src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java
        src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java
        src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockCompatibility.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java
        src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java

        Show
        Phabricator added a comment - heyongqiang updated the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Reviewers: JIRA, dhruba, tedyu, sc, mbautin address javadoc comments... REVISION DETAIL https://reviews.facebook.net/D2097 AFFECTED FILES src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockCompatibility.java src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12518814/HBASE-5521.D2097.10.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 15 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 164 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.mapreduce.TestImportTsv
        org.apache.hadoop.hbase.mapred.TestTableMapReduce
        org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1218//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1218//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1218//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12518814/HBASE-5521.D2097.10.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 164 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1218//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1218//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1218//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        tedyu has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        Looks good.

        I will defer the acceptance decision to Mikhail.

        INLINE COMMENTS
        src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:40 Should we say 'uncompressed' instead of 'not heavily compressed' here so that the javadoc is consistent with the name of the method ?

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - tedyu has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". Looks good. I will defer the acceptance decision to Mikhail. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java:40 Should we say 'uncompressed' instead of 'not heavily compressed' here so that the javadoc is consistent with the name of the method ? REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Phabricator added a comment -

        heyongqiang has commented on the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        i think Mikhail already did +1 on this. The last few updates are just to address tedyu's comments

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        Show
        Phabricator added a comment - heyongqiang has commented on the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". i think Mikhail already did +1 on this. The last few updates are just to address tedyu's comments REVISION DETAIL https://reviews.facebook.net/D2097
        Hide
        Phabricator added a comment -

        mbautin has accepted the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        BRANCH
        svn

        Show
        Phabricator added a comment - mbautin has accepted the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". REVISION DETAIL https://reviews.facebook.net/D2097 BRANCH svn
        Hide
        Mikhail Bautin added a comment -

        Attaching what has been committed.

        Show
        Mikhail Bautin added a comment - Attaching what has been committed.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12518927/HBASE-5521-jira-Move-compression-decompression-to-an-2012-03-19_12_12_32.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 19 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1225//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12518927/HBASE-5521-jira-Move-compression-decompression-to-an-2012-03-19_12_12_32.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 19 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1225//console This message is automatically generated.
        Hide
        Phabricator added a comment -

        mbautin has committed the revision "HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context".

        REVISION DETAIL
        https://reviews.facebook.net/D2097

        COMMIT
        https://reviews.facebook.net/rHBASE1302602

        Show
        Phabricator added a comment - mbautin has committed the revision " HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context". REVISION DETAIL https://reviews.facebook.net/D2097 COMMIT https://reviews.facebook.net/rHBASE1302602
        Hide
        Mikhail Bautin added a comment -

        Committed to trunk.

        Show
        Mikhail Bautin added a comment - Committed to trunk.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #143 (See https://builds.apache.org/job/HBase-TRUNK-security/143/)
        HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding
        context

        Author: Yongqiang He

        Summary:
        https://issues.apache.org/jira/browse/HBASE-5521

        As part of working on HBASE-5313, we want to add a new columnar encoder/decoder.
        It makes sense to move compression to be part of encoder/decoder:
        1) a scanner for a columnar encoded block can do lazy decompression to a
        specific part of a key value object
        2) avoid an extra bytes copy from encoder to hblock-writer.

        If there is no encoder specified for a writer, the HBlock.Writer will use a
        default compression-context to do something very similar to today's code.

        Test Plan: existing unit tests verified by mbautin and tedyu. And no new test
        added here since this code is just a preparation for columnar encoder. Will add
        testcase later in that diff.

        Reviewers: dhruba, tedyu, sc, mbautin

        Reviewed By: mbautin

        Differential Revision: https://reviews.facebook.net/D2097 (Revision 1302602)

        Result = FAILURE
        mbautin :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockCompatibility.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #143 (See https://builds.apache.org/job/HBase-TRUNK-security/143/ ) HBASE-5521 [jira] Move compression/decompression to an encoder specific encoding context Author: Yongqiang He Summary: https://issues.apache.org/jira/browse/HBASE-5521 As part of working on HBASE-5313 , we want to add a new columnar encoder/decoder. It makes sense to move compression to be part of encoder/decoder: 1) a scanner for a columnar encoded block can do lazy decompression to a specific part of a key value object 2) avoid an extra bytes copy from encoder to hblock-writer. If there is no encoder specified for a writer, the HBlock.Writer will use a default compression-context to do something very similar to today's code. Test Plan: existing unit tests verified by mbautin and tedyu. And no new test added here since this code is just a preparation for columnar encoder. Will add testcase later in that diff. Reviewers: dhruba, tedyu, sc, mbautin Reviewed By: mbautin Differential Revision: https://reviews.facebook.net/D2097 (Revision 1302602) Result = FAILURE mbautin : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/BufferedDataBlockEncoder.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/CopyKeyDataBlockEncoder.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoder.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/DataBlockEncoding.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/DiffKeyDeltaEncoder.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/EncodedDataBlock.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/FastDiffDeltaEncoder.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDecodingContext.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultEncodingContext.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockEncodingContext.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/encoding/PrefixKeyDeltaEncoder.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/Compression.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoder.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileWriterV2.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/NoOpDataBlockEncoder.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/encoding/TestDataBlockEncoders.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlock.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockCompatibility.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/DataBlockEncodingTool.java
        Hide
        stack added a comment -

        Marking closed.

        Show
        stack added a comment - Marking closed.

          People

          • Assignee:
            He Yongqiang
            Reporter:
            He Yongqiang
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development