[
https://issues.apache.org/jira/browse/HBASE-5521?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13231715#comment-13231715
]
Phabricator commented on HBASE-5521:
------------------------------------
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
> Move compression/decompression to an encoder specific encoding context
> ----------------------------------------------------------------------
>
> Key: HBASE-5521
> URL: https://issues.apache.org/jira/browse/HBASE-5521
> Project: HBase
> Issue Type: Improvement
> Reporter: He Yongqiang
> Assignee: He Yongqiang
> Attachments: HBASE-5521.1.patch, HBASE-5521.D2097.1.patch,
> HBASE-5521.D2097.2.patch, HBASE-5521.D2097.3.patch, HBASE-5521.D2097.4.patch,
> HBASE-5521.D2097.5.patch, HBASE-5521.D2097.6.patch, HBASE-5521.D2097.7.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
