> On 2010-10-01 14:44:53, stack wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java, line 65 > > <http://review.cloudera.org/r/925/diff/5/?file=13275#file13275line65> > > > > This needs to be data member? Its not just used once locally?
I copied from same way it's done for RS. It is used in constructor and then also in addMaster() method which is public and can be called by a client after initialization. > On 2010-10-01 14:44:53, stack wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java, line 141 > > <http://review.cloudera.org/r/925/diff/5/?file=13275#file13275line141> > > > > Why do this in constructor? Why not declare and allocate at same time? Again, copied from how it's done for RSs. Can move it into the declaration for both. > On 2010-10-01 14:44:53, stack wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java, line 224 > > <http://review.cloudera.org/r/925/diff/5/?file=13275#file13275line224> > > > > No biggie but I like the previous syntax Me too. I was smacked two years ago doing one-line ifs and such, told that HBase form was to always have braces. I had my IDE setup to not allow the former but have changed it now to permit one liners like this (when I hit code cleanup, it did this automatically). Will revert. > On 2010-10-01 14:44:53, stack wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java, line 267 > > <http://review.cloudera.org/r/925/diff/5/?file=13275#file13275line267> > > > > Can you read this from zk instead? Yes, but I don't think that would be very useful for unit tests since you'd be relying on correct behavior/interaction of masters w/ ZK. The way this is done is super simple... when a master becomes active, it flips a boolean. My thought was to try and make this as fool proof as possible. > On 2010-10-01 14:44:53, stack wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 217 > > <http://review.cloudera.org/r/925/diff/5/?file=13276#file13276line217> > > > > No need for this comment. CHeck the javadoc on this method. It used > > to be detailed steps IIRC. The detail no longer is correct. Revamped all comments. Check next diff. > On 2010-10-01 14:44:53, stack wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 322 > > <http://review.cloudera.org/r/925/diff/5/?file=13276#file13276line322> > > > > Does this if span too much? Could you test stopped before calling > > initialization? loop already does stopped? The if is immediately after we break out of blocking to become the active master. This check is for the case where we are backup master but then we are being shutdown. We get interrupted from blocking on becoming master, then we do this check. What is inside the if{} is the stuff we don't want to run (only run if you are actually active master). The stuff below the if is the stuff that is run in both cases of a master shutdown (backup master being shutdown, or active master being shutdown in master or cluster shutdown). Not the prettiest, tried a couple ways, but this ended up being simplest. Will add some comments. > On 2010-10-01 14:44:53, stack wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 345 > > <http://review.cloudera.org/r/925/diff/5/?file=13276#file13276line345> > > > > What services are started in teh constructor now? Changed comment. It's services that are started whether backup or active master. > On 2010-10-01 14:44:53, stack wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, > > line 674 > > <http://review.cloudera.org/r/925/diff/5/?file=13278#file13278line674> > > > > Do we lose messages here? Important we not drop split, etc. FYI, we > > need to do some more thought around split message and master failover. See > > over in master where we register servers on region server report for note. Shouldn't be any different than before. If we get IOE, we wouldn't have any messages returned from the method. Agree we need to more thought on this. Why piggyback this on heartbeat? Maybe direct RPC and master can return something that says success? If fail, then it'll retry from RS? > On 2010-10-01 14:44:53, stack wrote: > > trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java, > > line 133 > > <http://review.cloudera.org/r/925/diff/5/?file=13283#file13283line133> > > > > How about a test where there are RIT and master joins and assertion it > > does the fixup? > > > > There is white space above the 'yay'. That's the plan. > On 2010-10-01 14:44:53, stack wrote: > > trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java, > > line 86 > > <http://review.cloudera.org/r/925/diff/5/?file=13283#file13283line86> > > > > Yeah, whats this about? Will try and get rid of it all. The test is just a first go. I'd like to get this committed because HMaster and HRS keep changing and I have to rebase the changes. So I'm looking for a +1 to commit to get the changes in and then I'll continue working on fancier tests with this stuff. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/925/#review1378 ----------------------------------------------------------- On 2010-10-01 10:00:03, Jonathan Gray wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.cloudera.org/r/925/ > ----------------------------------------------------------- > > (Updated 2010-10-01 10:00:03) > > > Review request for hbase, stack, Karthik Ranganathan, and Kannan > Muthukkaruppan. > > > Summary > ------- > > To really be able to unit test the new master properly, we need to be able to > have multiple masters running at once within a single logical cluster. > > Also exposes methods to get the currently active master and can access each > individually in the same way that it's done for multiple RS. > > This is mostly duplicating what we do for RS but for masters. > > > This addresses bug HBASE-3053. > http://issues.apache.org/jira/browse/HBASE-3053 > > > Diffs > ----- > > trunk/src/main/java/org/apache/hadoop/hbase/HConstants.java 1003327 > trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1003327 > trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1003327 > trunk/src/main/java/org/apache/hadoop/hbase/master/HMasterCommandLine.java > 1003327 > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java > 1003327 > trunk/src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java > 1003327 > trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java > 1003327 > trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java > 1003327 > trunk/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 1003327 > trunk/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java > PRE-CREATION > > Diff: http://review.cloudera.org/r/925/diff > > > Testing > ------- > > Untested but compiles. This is first go at a patch. I'm going to work on > writing a new unit test that uses this next. > > > Thanks, > > Jonathan > >
