[
https://issues.apache.org/jira/browse/HBASE-4608?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13213316#comment-13213316
]
jirapos...@reviews.apache.org commented on HBASE-4608:
------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2740/#review5265
-----------------------------------------------------------
This looks great. Some small comments below.
src/main/java/org/apache/hadoop/hbase/regionserver/wal/Compressor.java
<https://reviews.apache.org/r/2740/#comment11488>
Should this javadoc here in the class include the notes you made for Kannan
where you describe how it all works? If not here, where else will doc. on how
the Compressor works go?
Maybe you should purge all mention of WAL from this class -- e.g.
WALDictionary -- because it seems like it could be easily generalized (I
suppose we can do that later).
src/main/java/org/apache/hadoop/hbase/regionserver/wal/Compressor.java
<https://reviews.apache.org/r/2740/#comment11489>
The way the usage is written, -u and -c are optional. You should fix that.
Looks like they are required going by fact that args.length needs to be 3.
Also, it looks like you take --help, the long form, or -u/-c the short forms.
Either take all short forms or take both long and short form to be consistent.
src/main/java/org/apache/hadoop/hbase/regionserver/wal/Compressor.java
<https://reviews.apache.org/r/2740/#comment11490>
Why is the tool called WALCompressor in the usage but the class I invoke is
Compressor?
src/main/java/org/apache/hadoop/hbase/regionserver/wal/Compressor.java
<https://reviews.apache.org/r/2740/#comment11491>
This does not need to be an HBaseConfiguration? There are no configs in
hbase-site.xml that might effect whats going on here?
src/main/java/org/apache/hadoop/hbase/regionserver/wal/Compressor.java
<https://reviews.apache.org/r/2740/#comment11492>
Doc the '@return'
src/main/java/org/apache/hadoop/hbase/regionserver/wal/Compressor.java
<https://reviews.apache.org/r/2740/#comment11493>
Doc the return
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java
<https://reviews.apache.org/r/2740/#comment11494>
White space
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java
<https://reviews.apache.org/r/2740/#comment11495>
When is this called? Post construction? Should it be part of constructor?
What happens if its called part way through the writing of a WAL? Will we
start compressing a WAL in the middle?
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogKey.java
<https://reviews.apache.org/r/2740/#comment11496>
I don't follow whats going on here. What happens when len >= 0? Why is it
< 0? Whats that mean? Whats v2 of hlogkey? What if keyContext is not null?
src/main/java/org/apache/hadoop/hbase/regionserver/wal/KeyValueCompression.java
<https://reviews.apache.org/r/2740/#comment11497>
Class comment on what this is about?
src/main/java/org/apache/hadoop/hbase/regionserver/wal/KeyValueCompression.java
<https://reviews.apache.org/r/2740/#comment11498>
Why do I clear this? Why not just throw it away? Does clearing make it so
I can recycle this instance?
src/main/java/org/apache/hadoop/hbase/regionserver/wal/LRUDictionary.java
<https://reviews.apache.org/r/2740/#comment11499>
Why would I ever let go of terms in the dictionary? Should you explain why
in class comment?
src/main/java/org/apache/hadoop/hbase/regionserver/wal/LRUDictionary.java
<https://reviews.apache.org/r/2740/#comment11501>
Should this be static? Does it need reference to outer class?
src/main/java/org/apache/hadoop/hbase/regionserver/wal/LRUDictionary.java
<https://reviews.apache.org/r/2740/#comment11502>
Class comment? Should this be static?
src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceFileLogReader.java
<https://reviews.apache.org/r/2740/#comment11503>
Why am I reading whether compression is on or off by looking at config?
Why am I not looking into head of the WAL file and figure its compressed and
then decompressing? Otherwise, if config is disabled but I'm fed a compressed
file, do I just burp? See the white space added here.
src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALDictionary.java
<https://reviews.apache.org/r/2740/#comment11504>
Should be just called Dictionary. Its in the wal package. No need of the
redundant prefix?
src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALReplayCompressed.java
<https://reviews.apache.org/r/2740/#comment11505>
This will run all the tests in TestWALReplay? Nice.
- Michael
On 2012-02-22 03:46:12, Li Pi wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2740/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-02-22 03:46:12)
bq.
bq.
bq. Review request for hbase, Eli Collins and Todd Lipcon.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. HLog compression. Has unit tests and a command line tool for
compressing/decompressing.
bq.
bq.
bq. This addresses bug HBase-4608.
bq. https://issues.apache.org/jira/browse/HBase-4608
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. src/main/java/org/apache/hadoop/hbase/HConstants.java 35339b6
bq. src/main/java/org/apache/hadoop/hbase/regionserver/wal/Compressor.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java c945a99
bq. src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogKey.java
f067221
bq.
src/main/java/org/apache/hadoop/hbase/regionserver/wal/KeyValueCompression.java
PRE-CREATION
bq.
src/main/java/org/apache/hadoop/hbase/regionserver/wal/LRUDictionary.java
PRE-CREATION
bq.
src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceFileLogReader.java
d9cd6de
bq.
src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceFileLogWriter.java
cbef70f
bq.
src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALDictionary.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALEdit.java
e1117ef
bq. src/main/java/org/apache/hadoop/hbase/util/Bytes.java ead9a3b
bq.
src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLRUDictionary.java
PRE-CREATION
bq.
src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALReplay.java
23d27fd
bq.
src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALReplayCompressed.java
PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/2740/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Li
bq.
bq.
> HLog Compression
> ----------------
>
> Key: HBASE-4608
> URL: https://issues.apache.org/jira/browse/HBASE-4608
> Project: HBase
> Issue Type: New Feature
> Reporter: Li Pi
> Assignee: Li Pi
> Attachments: 4608v1.txt, 4608v13.txt, 4608v13.txt, 4608v14.txt,
> 4608v5.txt, 4608v6.txt, 4608v7.txt, 4608v8fixed.txt
>
>
> The current bottleneck to HBase write speed is replicating the WAL appends
> across different datanodes. We can speed up this process by compressing the
> HLog. Current plan involves using a dictionary to compress table name, region
> id, cf name, and possibly other bits of repeated data. Also, HLog format may
> be changed in other ways to produce a smaller HLog.
--
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
