You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Karthik Ranganathan <ka...@gmail.com> on 2010/06/12 01:24:15 UTC
Review Request: HBASE-2694 Move RS to Master region open/close messaging
into ZooKeeper
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/171/
-----------------------------------------------------------
Review request for hbase, Todd Lipcon and stack.
Summary
-------
Moved all RS -> Master communication to go through ZK so that we can enable master failover.
This addresses bug HBASE-2694.
http://issues.apache.org/jira/browse/HBASE-2694
Diffs
-----
trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 953804
trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java PRE-CREATION
trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java PRE-CREATION
trunk/src/main/java/org/apache/hadoop/hbase/executor/NamedThreadFactory.java PRE-CREATION
trunk/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionEventData.java PRE-CREATION
trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 953804
trunk/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionClose.java 953804
trunk/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionOpen.java 953804
trunk/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionStatusChange.java 953804
trunk/src/main/java/org/apache/hadoop/hbase/master/RegionManager.java 953804
trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 953804
trunk/src/main/java/org/apache/hadoop/hbase/master/ZKMasterAddressWatcher.java 953804
trunk/src/main/java/org/apache/hadoop/hbase/master/ZKUnassignedWatcher.java PRE-CREATION
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/MasterCloseRegionHandler.java PRE-CREATION
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/MasterOpenRegionHandler.java PRE-CREATION
trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 953804
trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RSZookeeperUpdater.java PRE-CREATION
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/HQuorumPeer.java 953804
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java 953804
trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 953804
trunk/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java 953804
trunk/src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 953804
trunk/src/test/java/org/apache/hadoop/hbase/master/TestZKBasedCloseRegion.java PRE-CREATION
trunk/src/test/java/org/apache/hadoop/hbase/master/TestZKBasedReopenRegion.java PRE-CREATION
Diff: http://review.hbase.org/r/171/diff
Testing
-------
Unit tested.
Thanks,
Karthik
Re: Review Request: HBASE-2694 Move RS to Master region open/close messaging
into ZooKeeper
Posted by Jonathan Gray <jg...@apache.org>.
> On 2010-06-11 17:35:09, Todd Lipcon wrote:
> > Skimmed it reasonably quickly, looks good.
> > Major concern is how often we catch and just log exceptions. In most cases I think these are errors we can't recover from and it's best to throw RTE and shut down the whole server.
> >
> > Several places missing license headers.
Thanks for review Todd.
In ZK refactor, we will do in-depth look at where we should be throwing RTE vs logging/swallowing vs throwing.
When we do reimplementation of the open/close and all that, will look at the one you pointed out.
Applied the following changes per your comments:
- Switched to foreach in event handler
- Switch to NONE.value
- Added TODOs for using guava and reviewing IOE vs LOG
- Added license headers to ZKUnassignedWatcher, TestZKBasedReopenRegion, TestZKBasedCloseRegion
- Added logging to noop closing method
Will put final patch up onto JIRA and commit. Thanks!
> On 2010-06-11 17:35:09, Todd Lipcon wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/master/RegionManager.java, line 1000
> > <http://review.hbase.org/r/171/diff/1/?file=1267#file1267line1000>
> >
> > are these IOEs here and above really recoverable in any way? How could they happen?
They wouldn't happen unless the Writable implementations were broken. Not sure what we should be doing here besides logging? Better to log and ignore or throw RTE to bring attention to something being very broken?
Adding TODO to review what we should do here
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/171/#review202
-----------------------------------------------------------
On 2010-06-11 16:24:15, Karthik Ranganathan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/171/
> -----------------------------------------------------------
>
> (Updated 2010-06-11 16:24:15)
>
>
> Review request for hbase, Todd Lipcon and stack.
>
>
> Summary
> -------
>
> Moved all RS -> Master communication to go through ZK so that we can enable master failover.
>
>
> This addresses bug HBASE-2694.
> http://issues.apache.org/jira/browse/HBASE-2694
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/executor/NamedThreadFactory.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionEventData.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionClose.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionOpen.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionStatusChange.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/master/RegionManager.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/master/ZKMasterAddressWatcher.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/master/ZKUnassignedWatcher.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/MasterCloseRegionHandler.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/MasterOpenRegionHandler.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RSZookeeperUpdater.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/HQuorumPeer.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java 953804
> trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 953804
> trunk/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java 953804
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 953804
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestZKBasedCloseRegion.java PRE-CREATION
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestZKBasedReopenRegion.java PRE-CREATION
>
> Diff: http://review.hbase.org/r/171/diff
>
>
> Testing
> -------
>
> Unit tested.
>
>
> Thanks,
>
> Karthik
>
>
Re: Review Request: HBASE-2694 Move RS to Master region open/close messaging
into ZooKeeper
Posted by Todd Lipcon <to...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/171/#review202
-----------------------------------------------------------
Ship it!
Skimmed it reasonably quickly, looks good.
Major concern is how often we catch and just log exceptions. In most cases I think these are errors we can't recover from and it's best to throw RTE and shut down the whole server.
Several places missing license headers.
trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java
<http://review.hbase.org/r/171/#comment918>
use foreach form?
trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java
<http://review.hbase.org/r/171/#comment919>
foreach form
trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java
<http://review.hbase.org/r/171/#comment920>
== NONE.value, or if (this == NONE)?
trunk/src/main/java/org/apache/hadoop/hbase/executor/NamedThreadFactory.java
<http://review.hbase.org/r/171/#comment921>
You can just use this one:
http://guava-libraries.googlecode.com/svn/trunk/javadoc/com/google/common/util/concurrent/NamingThreadFactory.html
trunk/src/main/java/org/apache/hadoop/hbase/master/RegionManager.java
<http://review.hbase.org/r/171/#comment922>
are these IOEs here and above really recoverable in any way? How could they happen?
trunk/src/main/java/org/apache/hadoop/hbase/master/ZKUnassignedWatcher.java
<http://review.hbase.org/r/171/#comment923>
license
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/MasterOpenRegionHandler.java
<http://review.hbase.org/r/171/#comment924>
maybe add a LOG.debug here at least for now
trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.hbase.org/r/171/#comment925>
again, are these LOG.errors really recoverable things?
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/171/#comment926>
recoverable?
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
<http://review.hbase.org/r/171/#comment927>
can't wait til we refactor all this stuff
trunk/src/test/java/org/apache/hadoop/hbase/master/TestZKBasedCloseRegion.java
<http://review.hbase.org/r/171/#comment928>
license
- Todd
On 2010-06-11 16:24:15, Karthik Ranganathan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/171/
> -----------------------------------------------------------
>
> (Updated 2010-06-11 16:24:15)
>
>
> Review request for hbase, Todd Lipcon and stack.
>
>
> Summary
> -------
>
> Moved all RS -> Master communication to go through ZK so that we can enable master failover.
>
>
> This addresses bug HBASE-2694.
> http://issues.apache.org/jira/browse/HBASE-2694
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/executor/NamedThreadFactory.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionEventData.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionClose.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionOpen.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionStatusChange.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/master/RegionManager.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/master/ZKMasterAddressWatcher.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/master/ZKUnassignedWatcher.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/MasterCloseRegionHandler.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/MasterOpenRegionHandler.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/regionserver/RSZookeeperUpdater.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/HQuorumPeer.java 953804
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java 953804
> trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 953804
> trunk/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java 953804
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestMaster.java 953804
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestZKBasedCloseRegion.java PRE-CREATION
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestZKBasedReopenRegion.java PRE-CREATION
>
> Diff: http://review.hbase.org/r/171/diff
>
>
> Testing
> -------
>
> Unit tested.
>
>
> Thanks,
>
> Karthik
>
>