You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Alex Newman <ne...@cloudera.com> on 2010/07/23 02:25:12 UTC
Review Request: distributed log splitting now with better testing and
configurable splitting
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/370/
-----------------------------------------------------------
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
Re: Review Request: distributed log splitting now with better testing and
configurable splitting
Posted by Alex Newman <ne...@cloudera.com>.
> On 2010-07-23 12:01:53, Jean-Daniel Cryans wrote:
> > 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?
I'll add the story and get these changes in.
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/370/#review464
-----------------------------------------------------------
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
>
>
Re: Review Request: distributed log splitting now with better testing and
configurable splitting
Posted by Jean-Daniel Cryans <jd...@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
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
>
>
Re: Review Request: distributed log splitting now with better testing and
configurable splitting
Posted by Alex Newman <ne...@cloudera.com>.
> On 2010-07-23 13:41:09, Jonathan Gray wrote:
> > Hey Alex, this is looking good. The master rewrite branch has a refactoring of ZooKeeperWrapper and general ZK usage inside HBase that conflicts with this pretty significantly.
> >
> > Do you think you could pull the new methods and classes nested in ZooKeeperWrapper into a separate class of static methods? If you need the instantiated instance of ZKW, pass it in as the first argument to the static methods? That will make my life WAY easier when I have to merge the branch back into trunk.
> >
> > Also gives an opportunity to have a class comment in the new class explaining the overall usage of zk.
> >
> > Stuff like the names of the nodes can be left in the instantiated ZKW class since it makes sense to pull those in from the confs on instantiation.
> >
> > Cool? Let me know if you want an example.
sounds good
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/370/#review467
-----------------------------------------------------------
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
>
>
Re: Review Request: distributed log splitting now with better testing and
configurable splitting
Posted by Jonathan Gray <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/370/#review467
-----------------------------------------------------------
Hey Alex, this is looking good. The master rewrite branch has a refactoring of ZooKeeperWrapper and general ZK usage inside HBase that conflicts with this pretty significantly.
Do you think you could pull the new methods and classes nested in ZooKeeperWrapper into a separate class of static methods? If you need the instantiated instance of ZKW, pass it in as the first argument to the static methods? That will make my life WAY easier when I have to merge the branch back into trunk.
Also gives an opportunity to have a class comment in the new class explaining the overall usage of zk.
Stuff like the names of the nodes can be left in the instantiated ZKW class since it makes sense to pull those in from the confs on instantiation.
Cool? Let me know if you want an example.
- Jonathan
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
>
>