----------------------------------------------------------- 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 On 2010-07-22 17:25:12, Alex Newman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.hbase.org/r/370/ > ----------------------------------------------------------- > > (Updated 2010-07-22 17:25:12) > > > Review request for hbase. > > > Summary > ------- > > This build on the previous work. It does some smarter stuff with testing and > now splitting is configurable. > > > This addresses bug hbase-1364. > http://issues.apache.org/jira/browse/hbase-1364 > > > Diffs > ----- > > src/main/java/org/apache/hadoop/hbase/HConstants.java c77ebf5 > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java > f251d54 > src/main/java/org/apache/hadoop/hbase/regionserver/LogSplitter.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 5688c03 > src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java > 8225178 > src/main/resources/hbase-default.xml e3a9669 > > src/test/java/org/apache/hadoop/hbase/regionserver/wal/BaseTestHLogSplit.java > PRE-CREATION > > src/test/java/org/apache/hadoop/hbase/regionserver/wal/DistributedTestHLog.java > PRE-CREATION > > src/test/java/org/apache/hadoop/hbase/regionserver/wal/DistributedTestHLogSplit.java > PRE-CREATION > > src/test/java/org/apache/hadoop/hbase/regionserver/wal/DistributedTestHLogSplitSkipErrors.java > PRE-CREATION > > src/test/java/org/apache/hadoop/hbase/regionserver/wal/DistributedTestLogRolling.java > PRE-CREATION > src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java > ad8f9e5 > src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java > 908633e > > src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplitSkipErrors.java > PRE-CREATION > > src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogActionsListener.java > 776d78c > src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java > 9eae4b4 > src/test/resources/hbase-site.xml 3c0601a > > Diff: http://review.hbase.org/r/370/diff > > > Testing > ------- > > ran on our private hudson > > > Thanks, > > Alex > >
