You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Jonathan Gray <jg...@apache.org> on 2010/10/25 19:15:12 UTC
Review Request: TestRollingRestart
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1090/
-----------------------------------------------------------
Review request for hbase and stack.
Summary
-------
First go at a TestRollingRestart. Needs more work / harder tests per comments in the test.
Diffs
-----
trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1026935
trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java PRE-CREATION
Diff: http://review.cloudera.org/r/1090/diff
Testing
-------
Thanks,
Jonathan
Re: Review Request: TestRollingRestart
Posted by Jonathan Gray <jg...@apache.org>.
> On 2010-10-25 12:21:03, stack wrote:
> > Whats wrong w/ this? Looks good to me. Other tests would be nice but this does what rolling restart does now. Does it pass?
Nothing is wrong with it except that it passes. Your restarts have shown weird bugs and this isn't showing it (I don't have my proposed fix applied).
Need to add some concurrent RS stopping though rolling restart should not even do that.
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1090/#review1651
-----------------------------------------------------------
On 2010-10-25 10:15:12, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1090/
> -----------------------------------------------------------
>
> (Updated 2010-10-25 10:15:12)
>
>
> Review request for hbase and stack.
>
>
> Summary
> -------
>
> First go at a TestRollingRestart. Needs more work / harder tests per comments in the test.
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1026935
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java PRE-CREATION
>
> Diff: http://review.cloudera.org/r/1090/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan
>
>
Re: Review Request: TestRollingRestart
Posted by st...@duboce.net.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1090/#review1651
-----------------------------------------------------------
Whats wrong w/ this? Looks good to me. Other tests would be nice but this does what rolling restart does now. Does it pass?
- stack
On 2010-10-25 10:15:12, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1090/
> -----------------------------------------------------------
>
> (Updated 2010-10-25 10:15:12)
>
>
> Review request for hbase and stack.
>
>
> Summary
> -------
>
> First go at a TestRollingRestart. Needs more work / harder tests per comments in the test.
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1026935
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java PRE-CREATION
>
> Diff: http://review.cloudera.org/r/1090/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan
>
>
Re: Review Request: TestRollingRestart
Posted by st...@duboce.net.
> On 2010-10-28 13:43:01, stack wrote:
> > +1 Looks great. A nice little bomb thrown into the midst of new master.
Oh, reconcile this patch with the one I just posted here on rb. Add in the extra logging in close and open handlers in particular. Helps debugging knowing which handler for which server is running.
- stack
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1090/#review1688
-----------------------------------------------------------
On 2010-10-28 12:00:06, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1090/
> -----------------------------------------------------------
>
> (Updated 2010-10-28 12:00:06)
>
>
> Review request for hbase and stack.
>
>
> Summary
> -------
>
> First go at a TestRollingRestart. Needs more work / harder tests per comments in the test.
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 1027683
> trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 1027683
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java PRE-CREATION
>
> Diff: http://review.cloudera.org/r/1090/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan
>
>
Re: Review Request: TestRollingRestart
Posted by Jonathan Gray <jg...@apache.org>.
> On 2010-10-28 13:43:01, stack wrote:
> > +1 Looks great. A nice little bomb thrown into the midst of new master.
>
> stack wrote:
> Oh, reconcile this patch with the one I just posted here on rb. Add in the extra logging in close and open handlers in particular. Helps debugging knowing which handler for which server is running.
Will make your changes from below on commit. Thanks for review Stack!
> On 2010-10-28 13:43:01, stack wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java, line 259
> > <http://review.cloudera.org/r/1090/diff/4/?file=16161#file16161line259>
> >
> > You sure the RS you find here is same as the one you passed in? Do rst.getRegionServer.
In practice, it has always been the right one. I will add a check of HServerInfo that it's definitely same.
> On 2010-10-28 13:43:01, stack wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java, line 351
> > <http://review.cloudera.org/r/1090/diff/4/?file=16161#file16161line351>
> >
> > Ditto here.
K. Ditto here for me too.
> On 2010-10-28 13:43:01, stack wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java, line 430
> > <http://review.cloudera.org/r/1090/diff/4/?file=16162#file16162line430>
> >
> > This is fine but maybe we should make the equating sloppier... just do a contains 'Connection reset'?
Sure. Will do.
> On 2010-10-28 13:43:01, stack wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java, line 99
> > <http://review.cloudera.org/r/1090/diff/4/?file=16165#file16165line99>
> >
> > Isn't there an accessor for this? Why you need to make this non-private?
I'm using it in my unit test. This is how I know when the master has started and is done processing dead server. See TestRollingRestart. waitForRSShutdownToStartAndFinish()
> On 2010-10-28 13:43:01, stack wrote:
> > trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java, line 353
> > <http://review.cloudera.org/r/1090/diff/4/?file=16170#file16170line353>
> >
> > oh nelly
yeah this test is a fun one. i'm going to commit it @ 30 total regions but I have it passing at 1000 and killing my eclipse at 2000 but working up to the point of death.
> On 2010-10-28 13:43:01, stack wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 819
> > <http://review.cloudera.org/r/1090/diff/4/?file=16166#file16166line819>
> >
> > Add this string to the one above... to the 'Serving as' string.. its trying to be all vital statistics about an HRS post construction.
k.
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1090/#review1688
-----------------------------------------------------------
On 2010-10-28 12:00:06, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1090/
> -----------------------------------------------------------
>
> (Updated 2010-10-28 12:00:06)
>
>
> Review request for hbase and stack.
>
>
> Summary
> -------
>
> First go at a TestRollingRestart. Needs more work / harder tests per comments in the test.
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 1027683
> trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 1027683
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java PRE-CREATION
>
> Diff: http://review.cloudera.org/r/1090/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan
>
>
Re: Review Request: TestRollingRestart
Posted by Jonathan Gray <jg...@apache.org>.
> On 2010-10-28 13:43:01, stack wrote:
> > +1 Looks great. A nice little bomb thrown into the midst of new master.
>
> stack wrote:
> Oh, reconcile this patch with the one I just posted here on rb. Add in the extra logging in close and open handlers in particular. Helps debugging knowing which handler for which server is running.
>
> Jonathan Gray wrote:
> Will make your changes from below on commit. Thanks for review Stack!
Okay. This is enough changes to warrant another post to RB. Let me do the changes from your review, bring in your logging changes, and put up new diff on rb.
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1090/#review1688
-----------------------------------------------------------
On 2010-10-28 12:00:06, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1090/
> -----------------------------------------------------------
>
> (Updated 2010-10-28 12:00:06)
>
>
> Review request for hbase and stack.
>
>
> Summary
> -------
>
> First go at a TestRollingRestart. Needs more work / harder tests per comments in the test.
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 1027683
> trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 1027683
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java PRE-CREATION
>
> Diff: http://review.cloudera.org/r/1090/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan
>
>
Re: Review Request: TestRollingRestart
Posted by st...@duboce.net.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1090/#review1688
-----------------------------------------------------------
Ship it!
+1 Looks great. A nice little bomb thrown into the midst of new master.
trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
<http://review.cloudera.org/r/1090/#comment5586>
You sure the RS you find here is same as the one you passed in? Do rst.getRegionServer.
trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
<http://review.cloudera.org/r/1090/#comment5587>
Ditto here.
trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java
<http://review.cloudera.org/r/1090/#comment5588>
This is fine but maybe we should make the equating sloppier... just do a contains 'Connection reset'?
trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/1090/#comment5589>
This is important
trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
<http://review.cloudera.org/r/1090/#comment5590>
Good
trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<http://review.cloudera.org/r/1090/#comment5591>
good
trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
<http://review.cloudera.org/r/1090/#comment5592>
Isn't there an accessor for this? Why you need to make this non-private?
trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.cloudera.org/r/1090/#comment5593>
Good
trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.cloudera.org/r/1090/#comment5594>
Add this string to the one above... to the 'Serving as' string.. its trying to be all vital statistics about an HRS post construction.
trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java
<http://review.cloudera.org/r/1090/#comment5595>
oh nelly
- stack
On 2010-10-28 12:00:06, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1090/
> -----------------------------------------------------------
>
> (Updated 2010-10-28 12:00:06)
>
>
> Review request for hbase and stack.
>
>
> Summary
> -------
>
> First go at a TestRollingRestart. Needs more work / harder tests per comments in the test.
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java 1027683
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 1027683
> trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 1027683
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java PRE-CREATION
>
> Diff: http://review.cloudera.org/r/1090/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan
>
>
Re: Review Request: TestRollingRestart
Posted by st...@duboce.net.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1090/#review1692
-----------------------------------------------------------
Ship it!
+1
- stack
On 2010-10-28 14:04:59, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1090/
> -----------------------------------------------------------
>
> (Updated 2010-10-28 14:04:59)
>
>
> Review request for hbase and stack.
>
>
> Summary
> -------
>
> First go at a TestRollingRestart. Needs more work / harder tests per comments in the test.
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1028470
> trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 1028470
> trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1028470
> trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1028470
> trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 1028470
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ClosedRegionHandler.java 1028470
> trunk/src/main/java/org/apache/hadoop/hbase/master/handler/OpenedRegionHandler.java 1028470
> trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1028470
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java 1028470
> trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 1028470
> trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 1028470
> trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java PRE-CREATION
>
> Diff: http://review.cloudera.org/r/1090/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jonathan
>
>
Re: Review Request: TestRollingRestart
Posted by Jonathan Gray <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1090/
-----------------------------------------------------------
(Updated 2010-10-28 14:04:59.669378)
Review request for hbase and stack.
Changes
-------
Stack's suggestions and two log line changes from his patch.
Summary
-------
First go at a TestRollingRestart. Needs more work / harder tests per comments in the test.
Diffs (updated)
-----
trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1028470
trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 1028470
trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1028470
trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1028470
trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 1028470
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ClosedRegionHandler.java 1028470
trunk/src/main/java/org/apache/hadoop/hbase/master/handler/OpenedRegionHandler.java 1028470
trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1028470
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java 1028470
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 1028470
trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 1028470
trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java PRE-CREATION
Diff: http://review.cloudera.org/r/1090/diff
Testing
-------
Thanks,
Jonathan
Re: Review Request: TestRollingRestart
Posted by Jonathan Gray <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1090/
-----------------------------------------------------------
(Updated 2010-10-28 12:00:06.042321)
Review request for hbase and stack.
Changes
-------
Includes small fixes discussed w/ stack.
Summary
-------
First go at a TestRollingRestart. Needs more work / harder tests per comments in the test.
Diffs (updated)
-----
trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1027683
trunk/src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java 1027683
trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 1027683
trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 1027683
trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 1027683
trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1027683
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java 1027683
trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 1027683
trunk/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 1027683
trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java PRE-CREATION
Diff: http://review.cloudera.org/r/1090/diff
Testing
-------
Thanks,
Jonathan
Re: Review Request: TestRollingRestart
Posted by Jonathan Gray <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1090/
-----------------------------------------------------------
(Updated 2010-10-26 00:01:59.924660)
Review request for hbase and stack.
Changes
-------
Adds more testing which kills the server hosting META 3 times in a row.
Summary
-------
First go at a TestRollingRestart. Needs more work / harder tests per comments in the test.
Diffs (updated)
-----
trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1027365
trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java PRE-CREATION
Diff: http://review.cloudera.org/r/1090/diff
Testing
-------
Thanks,
Jonathan
Re: Review Request: TestRollingRestart
Posted by Jonathan Gray <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1090/
-----------------------------------------------------------
(Updated 2010-10-25 14:02:19.371332)
Review request for hbase and stack.
Changes
-------
Adds more killingz of more serverz. Now it fails.
Summary
-------
First go at a TestRollingRestart. Needs more work / harder tests per comments in the test.
Diffs (updated)
-----
trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 1026935
trunk/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java PRE-CREATION
Diff: http://review.cloudera.org/r/1090/diff
Testing
-------
Thanks,
Jonathan