> On Oct. 9, 2012, 9:47 p.m., fpj wrote: > > Great job, Stu. I have just a few minor points.
Thanks for the review! > On Oct. 9, 2012, 9:47 p.m., fpj wrote: > > hedwig-server/src/test/java/org/apache/hedwig/server/subscriptions/TestUpdateSubscriptionState.java, > > line 44 > > <https://reviews.apache.org/r/7479/diff/1/?file=174774#file174774line44> > > > > Any particular reason for dropping it to 10? It was 100 before. It wasn't at all related to the rest of this patch, but a 100 second value here was causing a 100 second sleep in this test, which just seemed excessive. > On Oct. 9, 2012, 9:47 p.m., fpj wrote: > > hedwig-server/src/test/java/org/apache/hedwig/server/integration/TestHedwigRegion.java, > > line 228 > > <https://reviews.apache.org/r/7479/diff/1/?file=174771#file174771line228> > > > > Same question about being null. I'm just wondering why this check was > > not here before. It used to be that stopping a region entirely did not also explicitly kill your client... I actually have no idea how it worked before: if all hubs for the region are stopped, I would have expected this loop to fail for that region. After this patch, the client is explicitly killed with the region for stopRegion(x). > On Oct. 9, 2012, 9:47 p.m., fpj wrote: > > hedwig-server/src/test/java/org/apache/hedwig/server/integration/TestHedwigRegion.java, > > line 217 > > <https://reviews.apache.org/r/7479/diff/1/?file=174771#file174771line217> > > > > Why would client be null here? See below. > On Oct. 9, 2012, 9:47 p.m., fpj wrote: > > hedwig-server/src/test/java/org/apache/hedwig/server/HedwigRegionTestBase.java, > > line 47 > > <https://reviews.apache.org/r/7479/diff/1/?file=174767#file174767line47> > > > > I was thinking that it might be a good idea to add some comments here > > about how to use this base class. These comments would be to help new > > developers to write tests. What do you think? Will do. - Stu ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7479/#review12289 ----------------------------------------------------------- On Oct. 8, 2012, 8:57 p.m., Stu Hood wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7479/ > ----------------------------------------------------------- > > (Updated Oct. 8, 2012, 8:57 p.m.) > > > Review request for bookkeeper, Ivan Kelly and Aniruddha Laud. > > > Description > ------- > > https://issues.apache.org/jira/browse/BOOKKEEPER-423 > > > This addresses bug BOOKKEEPER-423. > https://issues.apache.org/jira/browse/BOOKKEEPER-423 > > > Diffs > ----- > > hedwig-server/src/test/java/org/apache/hedwig/server/HedwigHubTestBase.java > 523ecdc > > hedwig-server/src/test/java/org/apache/hedwig/server/HedwigRegionTestBase.java > 4ec0d50 > > hedwig-server/src/test/java/org/apache/hedwig/server/delivery/TestThrottlingDelivery.java > 66e9ff3 > > hedwig-server/src/test/java/org/apache/hedwig/server/filter/TestMessageFilter.java > ae46be2 > > hedwig-server/src/test/java/org/apache/hedwig/server/integration/TestHedwigHub.java > 02b4503 > > hedwig-server/src/test/java/org/apache/hedwig/server/integration/TestHedwigRegion.java > 0b1851e > > hedwig-server/src/test/java/org/apache/hedwig/server/persistence/MessageBoundedPersistenceTest.java > 03b40f3 > > hedwig-server/src/test/java/org/apache/hedwig/server/persistence/TestDeadlock.java > 114e0c5 > > hedwig-server/src/test/java/org/apache/hedwig/server/subscriptions/TestUpdateSubscriptionState.java > d21eef3 > > hedwig-server/src/test/java/org/apache/hedwig/server/topics/TestConcurrentTopicAcquisition.java > 956d31c > > Diff: https://reviews.apache.org/r/7479/diff/ > > > Testing > ------- > > > Thanks, > > Stu Hood > >
