hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jean-Daniel Cryans" <jdcry...@apache.org>
Subject Re: Review Request: HBASE-2697 The master rewrite
Date Tue, 03 Aug 2010 21:35:07 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/484/#review636
-----------------------------------------------------------


First pass, still trying to understand what's going on


branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java
<http://review.cloudera.org/r/484/#comment2393>

    unused?



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaEditor.java
<http://review.cloudera.org/r/484/#comment2383>

    Possible NPE



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaEditor.java
<http://review.cloudera.org/r/484/#comment2388>

    Possible NPE, server can be null



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaEditor.java
<http://review.cloudera.org/r/484/#comment2384>

    Possible NPE



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaEditor.java
<http://review.cloudera.org/r/484/#comment2385>

    Possible NPE



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
<http://review.cloudera.org/r/484/#comment2386>

    Possible NPE



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
<http://review.cloudera.org/r/484/#comment2387>

    Possible NPE, metaServer can be null



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
<http://review.cloudera.org/r/484/#comment2389>

    Possible NPE



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
<http://review.cloudera.org/r/484/#comment2397>

    javadoc for this method and the next one



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
<http://review.cloudera.org/r/484/#comment2401>

    This method and a few other ones in this class looks almost the same, I think there's
a refactoring potential



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
<http://review.cloudera.org/r/484/#comment2390>

    Possible NPE



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
<http://review.cloudera.org/r/484/#comment2398>

    Somehow this line looks weird to me, not sure if it's how you build the row or that you
don't use a final for ",,"



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
<http://review.cloudera.org/r/484/#comment2391>

    NPE



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
<http://review.cloudera.org/r/484/#comment2392>

    NPE



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
<http://review.cloudera.org/r/484/#comment2402>

    could enclose in a ifDebugEnabled



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/484/#comment2404>

    Javadoc looks anemic compared to the importance of that class



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/484/#comment2405>

    Could you handle if regionState is null at this level?



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/484/#comment2406>

    Is this code coming back later?



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/484/#comment2407>

    Looks fishy, what happens if anyone uses that state outside of that method?



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/484/#comment2409>

    Any investigation needed?



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/484/#comment2410>

    What happens if it never returns? Also I wonder if it's feasible that the assignment somehow
ends up waiting on itself because of region server failure



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/handler/CloseRegionHandler.java
<http://review.cloudera.org/r/484/#comment2412>

    I see red boxes :)



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java
<http://review.cloudera.org/r/484/#comment2413>

    red


- Jean-Daniel


On 2010-08-03 05:02:49, Jonathan Gray wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/484/
> -----------------------------------------------------------
> 
> (Updated 2010-08-03 05:02:49)
> 
> 
> Review request for hbase, stack and Karthik Ranganathan.
> 
> 
> Summary
> -------
> 
> This is basically the master rewrite.  It's massive, I apologize.  I can start a cluster,
create a table, insert a row, and get the row.  Restarting should work too.
> 
> I completely ripped out BaseScanner/MetaScanner as well as the RegionServerOperationQueue
stuff.  I'm sold on the former but there's a bit of work to fully replace the latter though
I think it's a desirable direction to get rid of it.  The RSOQ stuff has been replaced with
EventHandlers.  The BaseScanner is no longer necessary because we are not utilizing META to
determine when things are unassigned.
> 
> All the table operations (enable/disable, create/delete, addcolumn/del/modify) have been
reimplemented in handlers.
> 
> All META/ROOT server access is done through the new CatalogManager.  This uses ZK and
ROOT/META to keep active track of the current locations of the catalog regions.  A bit more
work needs to be done on defining our retry semantics for different operations.
> 
> The actual read/write operations on ROOT/META are done with new classes RootEditor, MetaEditor,
and MetaReader.  A lot of this code was scattered around HMaster, RegionManager, ServerManager,
HRegion, etc...
> 
> Some new methods in FileSystemManager, collected from being scattered around.
> 
> This also does final straw to old ZKWrapper and removes it.  All ZK stuff is going through
the new stuff.
> 
> Open/close is now going via RPC and ZK, no longer piggybacking any of these messages
on heartbeats (and with splits not working right now, even that is gone, so there will be
nothing it seems)
> 
> RegionManager was completely removed and replaced with a shiny, brand new AssignmentManager.
 It's already managed to get a bit hectic but is pretty straightforward.  It is the heart
of zk-based region assignment and most logic is in here.  It contains regionsInTransition,
the region plans, etc...
> 
> I added a new /disabled node in ZK to keep track of tables that are disabled.  I think
we need this to be able to reliably do disables across failures.
> 
> Among other changes....
> 
> 
> Here's my rough list of things left to do (covers most but not all TODOs in the branch)
> 
> 
> * make final decisions on root/meta timeouts.  almost everyone is coordinating
>   access through CatalogTracker which should make it easy to standardize.
>   if there are operations that should just retry indefinitely, they need to
>   resubmit themselves to their executor service.
> 
> * move splits to RS side, integrate new patch from stack on trunk
>   might need a new CREATED unassigned now, or new rpc, but get rid of sending
>   split notification on heartbeat?
>   how to handle splits concurrent with disable?
> 
> * review master startup order
> 
> * figure what to do with client table admin ops (flush, split, compact)
> 
> * MasterAddressManager move to extending ZooKeeperNodeTracker
> 
> * on region open (and wherever split children notify master) should check if
>   if the table is disabled and should close the regions... maybe.
> 
> * regionserver exit needs to be reimplemented in servermanager
>   also rs expiration
> 
> * add priorities and pool size to handlers (bring in from flush patch)
> 
> * in RootEditor there is a race condition between delete and watch?
> 
> * migrate TestZKBased* tests to use new handlers
> 
> * make sync calls for enable/disable (check and verify methods?)
> 
> * integrate load balancing
> 
> * finish TODOs on new failover path and remove old code in joinExistingCluster()
> 
> * finish TODOs on timeout monitor in assignmentmanager
> 
> * review filesystemmanager calls
> 
> * figure how to handle the very rare but possible race condition where two
>   RSs will update META and the later one can squash the valid one if there was
>   a long gc pause
> 
> * synchronize all access to the boolean in ActiveMasterManager
>   (now this is probably just move it to extend ZKNodeTracker)
> 
> * there are some races with master wanting to connect for rpc
>   to regionserver and the rs starting its rpc server, need to address
> 
> * migrate TestMasterTransitions or make new?
> 
> * fix or remove last couple master tests that used RSOQ
> 
> * write new tests!!!
> 
> 
> This addresses bug HBASE-2697.
>     http://issues.apache.org/jira/browse/HBASE-2697
> 
> 
> Diffs
> -----
> 
>   branches/0.90_master_rewrite/BRANCH_TODO.txt 979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/Abortable.java 979909

>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ClusterStatus.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/HRegionInfo.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/HServerInfo.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/InvalidFamilyException.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ServerController.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaEditor.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/catalog/RootEditor.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AddColumn.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/BaseScanner.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ChangeTableState.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ColumnOperation.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/DeleteColumn.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/FileSystemManager.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/MasterController.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/MasterStatus.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/MetaRegion.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/MetaScanner.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ModifyColumn.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ModifyTableMeta.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionClose.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionOpen.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionStatusChange.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ProcessServerShutdown.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/RegionManager.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/RegionServerOperation.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/RegionServerOperationListener.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/RegionServerOperationQueue.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/RetryableMetaOperation.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/RootScanner.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/TableDelete.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/TableOperation.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/ClosedRegionHandler.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/DisableTableHandler.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/EnableTableHandler.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/MasterCloseRegionHandler.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/MasterOpenRegionHandler.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/ModifyTableHandler.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/OpenedRegionHandler.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableAddFamilyHandler.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableDeleteFamilyHandler.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableEventHandler.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/TableModifyFamilyHandler.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplitThread.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/MasterAddressManager.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/RSZookeeperUpdater.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerController.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/handler/CloseRegionHandler.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/MetaNodeTracker.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKTableDisable.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperListener.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperNodeTracker.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java
979909 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java
979909 
>   branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
979909 
>   branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java
979909 
>   branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestMaster.java
979909 
>   branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestRestartCluster.java
979909 
> 
> Diff: http://review.cloudera.org/r/484/diff
> 
> 
> Testing
> -------
> 
> simple cluster tests passing, need to run (and write) more tests
> 
> 
> Thanks,
> 
> Jonathan
> 
>


Mime
View raw message