[
https://issues.apache.org/jira/browse/HBASE-1364?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12891747#action_12891747
]
HBase Review Board commented on HBASE-1364:
-------------------------------------------
Message from: "Jean-Daniel Cryans" <jdcry...@apache.org>
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/370/#review464
-----------------------------------------------------------
First pass on this patch. Lots of cleanup that needs to be done, and it's a bit
hard to follow the flow of events without any clear documentation that gives an
overview of distributed splitting. Nothing big, just some use cases that could
be put in the class javadoc of LogSplitter?
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.hbase.org/r/370/#comment1903>
I'm sure you have a good reason of putting that there, but at least one
issue I'm seeing is that this code is also in init() (which will be run just
after that) and it's almost the same thing.
Also, fs.automatic.close is handled by the ShutdownHook class, you
shouldn't be setting it.
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.hbase.org/r/370/#comment1904>
Fix those long lines.
src/main/java/org/apache/hadoop/hbase/regionserver/LogSplitter.java
<http://review.hbase.org/r/370/#comment1905>
rogue "q"
src/main/java/org/apache/hadoop/hbase/regionserver/LogSplitter.java
<http://review.hbase.org/r/370/#comment1914>
Why are those static?
src/main/java/org/apache/hadoop/hbase/regionserver/LogSplitter.java
<http://review.hbase.org/r/370/#comment1906>
remove that white space and all the others in that class at the same place
src/main/java/org/apache/hadoop/hbase/regionserver/LogSplitter.java
<http://review.hbase.org/r/370/#comment1922>
both process and run call this method, can there be a race?
src/main/java/org/apache/hadoop/hbase/regionserver/LogSplitter.java
<http://review.hbase.org/r/370/#comment1923>
don't need to declare this here
src/main/java/org/apache/hadoop/hbase/regionserver/LogSplitter.java
<http://review.hbase.org/r/370/#comment1907>
What does that mean?
src/main/java/org/apache/hadoop/hbase/regionserver/LogSplitter.java
<http://review.hbase.org/r/370/#comment1918>
why two lines?
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java
<http://review.hbase.org/r/370/#comment1908>
rogue "b"
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java
<http://review.hbase.org/r/370/#comment1909>
?
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1917>
confusing name when looking at what's returned, fix that
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1921>
Why two lines for nodes? Also, if nodes is null for any reason, won't that
throw an NPE?
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1924>
most of that stuff can be removed and put into the
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1928>
so you create a lock with data=null?
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1930>
Or you were just disconnected, could mean a lot of things right?
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1915>
JavaBean convention, don't start parameters' name with upper case
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1916>
So we log here and we log in LogSplitter, remove one of them?
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1933>
again, name confusing WRT returned type
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1931>
same comment
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1934>
don't start with upper case
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1938>
Usually ppl check that the other way around
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1936>
use HConstants.EMPTY_BYTE_ARRAY
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1940>
third ERROR line if splitPath is null, keep only one
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1943>
pull the next lines on this one with a tertiary operator
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1941>
EMPTY_BYTE_ARRAY
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1942>
Use Bytes.toBytes
src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/370/#comment1944>
Use Bytes.toString
src/test/java/org/apache/hadoop/hbase/regionserver/wal/DistributedTestHLog.java
<http://review.hbase.org/r/370/#comment1911>
copy pasta, we're in 2010 now! :P
src/test/java/org/apache/hadoop/hbase/regionserver/wal/DistributedTestHLog.java
<http://review.hbase.org/r/370/#comment1910>
clean
src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java
<http://review.hbase.org/r/370/#comment1912>
white spaces...
src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java
<http://review.hbase.org/r/370/#comment1913>
clean
- Jean-Daniel
> [performance] Distributed splitting of regionserver commit logs
> ---------------------------------------------------------------
>
> Key: HBASE-1364
> URL: https://issues.apache.org/jira/browse/HBASE-1364
> Project: HBase
> Issue Type: Improvement
> Reporter: stack
> Assignee: Alex Newman
> Priority: Critical
> Fix For: 0.92.0
>
> Attachments: 1 (3), 1364-v2.patch, 1364.patch
>
> Time Spent: 8h
> Remaining Estimate: 0h
>
> HBASE-1008 has some improvements to our log splitting on regionserver crash;
> but it needs to run even faster.
> (Below is from HBASE-1008)
> In bigtable paper, the split is distributed. If we're going to have 1000
> logs, we need to distribute or at least multithread the splitting.
> 1. As is, regions starting up expect to find one reconstruction log only.
> Need to make it so pick up a bunch of edit logs and it should be fine that
> logs are elsewhere in hdfs in an output directory written by all split
> participants whether multithreaded or a mapreduce-like distributed process
> (Lets write our distributed sort first as a MR so we learn whats involved;
> distributed sort, as much as possible should use MR framework pieces). On
> startup, regions go to this directory and pick up the files written by split
> participants deleting and clearing the dir when all have been read in. Making
> it so can take multiple logs for input, can also make the split process more
> robust rather than current tenuous process which loses all edits if it
> doesn't make it to the end without error.
> 2. Each column family rereads the reconstruction log to find its edits. Need
> to fix that. Split can sort the edits by column family so store only reads
> its edits.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
