You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org> on 2012/03/01 10:54:01 UTC
[jira] [Commented] (HBASE-4608) HLog Compression
[ https://issues.apache.org/jira/browse/HBASE-4608?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13219947#comment-13219947 ]
jiraposter@reviews.apache.org commented on HBASE-4608:
------------------------------------------------------
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/Compressor.java, line 37
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78622#file78622line37>
bq. >
bq. > 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?
bq. >
bq. > 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).
Included!
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/Compressor.java, line 47
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78622#file78622line47>
bq. >
bq. > 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.
System.out.println("Exactly one of -u or -c must be specified"); should take care of the required thing.
Help now takes both short and long forms. Everything else just takes short forms.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/Compressor.java, line 66
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78622#file78622line66>
bq. >
bq. > Why is the tool called WALCompressor in the usage but the class I invoke is Compressor?
Probably should be called compressor.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/Compressor.java, line 79
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78622#file78622line79>
bq. >
bq. > This does not need to be an HBaseConfiguration? There are no configs in hbase-site.xml that might effect whats going on here?
Not really. All that matters is whether compression is on or off.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/Compressor.java, line 108
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78622#file78622line108>
bq. >
bq. > Doc the '@return'
fixed.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/Compressor.java, line 141
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78622#file78622line141>
bq. >
bq. > Doc the return
fixed.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java, line 1671
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78623#file78623line1671>
bq. >
bq. > White space
fixed.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java, line 1675
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78623#file78623line1675>
bq. >
bq. > 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?
Its called when an logwriter is created. We will start compression a log in the middle if we happen to call it at that time. But that shouldn't happen.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogKey.java, line 270
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78624#file78624line270>
bq. >
bq. > 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?
HLogKey has two different formats. If len < 0, that means we're reading the old version of the HLog.
Keycontext is the compression context that holds the dictionaries used in compression. If it isn't null, that means compression is enabled.
If len > 0, we're on version 1. We can't compress version 1, but the code for reading version 1 is still in there, for transitioning from earlier HLogs. Compression should never be enabled if we're reading in version 1 Hlogs, because there shouldn't be any version 1 hlogs.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/KeyValueCompression.java, line 119
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78625#file78625line119>
bq. >
bq. > Class comment on what this is about?
Just a tuple class for holding the various dictionaries used in compression.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/KeyValueCompression.java, line 141
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78625#file78625line141>
bq. >
bq. > Why do I clear this? Why not just throw it away? Does clearing make it so I can recycle this instance?
Correct. We clear it so we can recycle this instance instead of having to create a new dictionary. Not sure if this makes a huge difference in terms of performance.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/LRUDictionary.java, line 29
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78626#file78626line29>
bq. >
bq. > Why would I ever let go of terms in the dictionary? Should you explain why in class comment?
We let go of terms in the dictionary since we have only an finite amount of space, and ability to reference terms of the dictionary.
If we're using a 2 byte key, that limits our reference space to 65536. We could end up using vints for entries into the dictionary, but this could end up with it growing pretty huge.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/LRUDictionary.java, line 64
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78626#file78626line64>
bq. >
bq. > Should this be static? Does it need reference to outer class?
It doesn't need to reference the outer class. Made static.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/LRUDictionary.java, line 168
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78626#file78626line168>
bq. >
bq. > Class comment? Should this be static?
made static.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceFileLogReader.java, line 176
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78627#file78627line176>
bq. >
bq. > 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.
We just burp if compression is on and we get fed an uncompressed file. This should be easy to change though - on the read side.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALDictionary.java, line 28
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78629#file78629line28>
bq. >
bq. > Should be just called Dictionary. Its in the wal package. No need of the redundant prefix?
Sure. But we have WALActionsListener and a bunch of other things starting with WAL. I figured we can just have that as well.
Renamed to dictionary.
bq. On 2012-02-22 05:11:37, Michael Stack wrote:
bq. > src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALReplayCompressed.java, line 38
bq. > <https://reviews.apache.org/r/2740/diff/19/?file=78634#file78634line38>
bq. >
bq. > This will run all the tests in TestWALReplay? Nice.
Yup. thats exactly what it does.
- Li
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2740/#review5265
-----------------------------------------------------------
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