hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Todd Lipcon" <t...@cloudera.com>
Subject Re: Review Request: HBASE-2695/HBASE-2696 The rest of it. Master fully on new ZK stuff.
Date Wed, 28 Jul 2010 19:52:13 GMT

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



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java
<http://review.cloudera.org/r/387/#comment2070>

    could you make this just be a field of the enum, and set it on in the enum constructor?
    
    RS2ZK_REGION_CLOSING(1, "CLOSING")
    
    the switch (this) just seems a little ugly. no big deal though.



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
<http://review.cloudera.org/r/387/#comment2071>

    I guess it's out of scope, but I'd love it if these were moved to avro data, rather than
Writables - it will really help with debuggability since we could tell it to use JSON serialization,
and then we can easily inspect ZK using external tools without having to write them ourselves.



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
<http://review.cloudera.org/r/387/#comment2073>

    add assert?



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
<http://review.cloudera.org/r/387/#comment2074>

    assert?
    
    also consider maybe making these into factory methods to clarify their usage?



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
<http://review.cloudera.org/r/387/#comment2075>

    don't we usually capitalize this as Timestamp? for some reason this looks weird.



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
<http://review.cloudera.org/r/387/#comment2077>

    else serverName = null



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
<http://review.cloudera.org/r/387/#comment2078>

    else hmsg = null



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

    maybe add a boolean started=true, and assert to make sure we don't start twice, and checks
elsewhere to make sure it gets started?



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

    get result and assert it's empty, perhaps?



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

    default: log it?



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

    when would this happen?



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

    again, when?



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ClusterStatusTracker.java
<http://review.cloudera.org/r/387/#comment2085>

    UP_DATA



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
<http://review.cloudera.org/r/387/#comment2086>

    assert path.startsWith(zkw.assignmentZNode)



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
<http://review.cloudera.org/r/387/#comment2087>

    synchronizing on someone else's member seems error-prone here



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
<http://review.cloudera.org/r/387/#comment2090>

    is it possible to attach a string to this exception?



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
<http://review.cloudera.org/r/387/#comment2092>

    should we throw exceptions instead of returning false for stuff like this? I don't know
how exceptional this case is.



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
<http://review.cloudera.org/r/387/#comment2093>

    again on returning false here - maybe an exception like UnexpectedZNodeStateException()
so we can attach these nice warnings to the exception trace?



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
<http://review.cloudera.org/r/387/#comment2094>

    this duality of argument is a little strange.



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
<http://review.cloudera.org/r/387/#comment2095>

    do these synchronized blocks present possible deadlock issues?
    
    I'm wondering about something like this:
    
    Thread A:
    synchronized (nodes) {
      do some ZK op that starts a watch;
      do some synchronous ZK op;
    }
    
    ZK thread:
      get event that triggers watch
        call watch handler
          watch handler synchronizes on nodes and blocks
      ... blocked, so doesn't process further, so the above synchronous ZK op never returns
    
    



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
<http://review.cloudera.org/r/387/#comment2096>

    typo: there



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperNodeTracker.java
<http://review.cloudera.org/r/387/#comment2097>

    I think wait() on a public object is bad form... can you use an internal Object to wait/notify
on, to be clear what the condition is you're waiting for?
    
    also maybe best to wait(1000); "just in case"



branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java
<http://review.cloudera.org/r/387/#comment2098>

    hm?



branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestMasterTransitions.java
<http://review.cloudera.org/r/387/#comment2099>

    TODO to re-enable?



branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/zookeeper/TestZooKeeperNodeTracker.java
<http://review.cloudera.org/r/387/#comment2100>

    perhaps add LOGs to these stubs for handy debugging of test failures?


- Todd


On 2010-07-26 18:57:08, Jonathan Gray wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/387/
> -----------------------------------------------------------
> 
> (Updated 2010-07-26 18:57:08)
> 
> 
> Review request for hbase, stack and Karthik Ranganathan.
> 
> 
> Summary
> -------
> 
> This is the rest of the master cleanup and zookeeper cleanup.  Everything is moved over
to the new ZooKeeperWatcher, ZooKeeperListeners, ZKUtil/ZKAssign, etc...
> 
> There is a second page to the diff linked at the bottom with lots of good stuff, don't
miss it!
> 
> Now on to the good stuff!
> 
> 
> This addresses bugs HBASE-2695 and HBASE-2696.
>     http://issues.apache.org/jira/browse/HBASE-2695
>     http://issues.apache.org/jira/browse/HBASE-2696
> 
> 
> Diffs
> -----
> 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/Abortable.java PRE-CREATION

>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/MiniZooKeeperCluster.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/ServerController.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/HBaseEventHandler.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/HBaseExecutorService.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionEventData.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ProcessRegionOpen.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/RegionManager.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/ZKUnassignedWatcher.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/MasterCloseRegionHandler.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/master/handler/MasterOpenRegionHandler.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/MasterAddressManager.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/regionserver/RSZookeeperUpdater.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ClusterStatusTracker.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/RegionServerTracker.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
964617 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperNodeTracker.java
PRE-CREATION 
>   branches/0.90_master_rewrite/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java
964617 
>   branches/0.90_master_rewrite/src/main/resources/hbase-webapps/master/master.jsp 964617

>   branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java
964617 
>   branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/TestMultiParallelPut.java
964617 
>   branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/OOMEHMaster.java
964617 
>   branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestActiveMasterManager.java
964617 
>   branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestMasterTransitions.java
964617 
>   branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/master/TestRestartCluster.java
964617 
>   branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/regionserver/TestMasterAddressManager.java
964617 
>   branches/0.90_master_rewrite/src/test/java/org/apache/hadoop/hbase/zookeeper/TestZooKeeperNodeTracker.java
PRE-CREATION 
> 
> Diff: http://review.cloudera.org/r/387/diff
> 
> 
> Testing
> -------
> 
> Most unit tests passing.  Still addressing remaining failures but most seem to be related
to the fact that I was running multiple tests and ZK clusters were stomping on each other.
> 
> 
> Thanks,
> 
> Jonathan
> 
>


Mime
View raw message