----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1298/#review2083 -----------------------------------------------------------
Ship it! a few small comments. i think the loop should change as described in my comment (busy loop w/ call to currentTimeMillis as i read it). otherwise +1, good stuff. we need some tickle util class soon :) trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java <http://review.cloudera.org/r/1298/#comment6529> "on this server" should probably be left in comment to be clear what this is checking trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java <http://review.cloudera.org/r/1298/#comment6530> We were not previously but we should probably log this condition trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java <http://review.cloudera.org/r/1298/#comment6531> This is a busy wait loop? Should we add a wait/notify on something passed to the thread and w/ a timeout of the period? And then we should probably also have some kind of max timeout. Even if minutes, there could be weird cluster state where the RS misses META availability but someone else might handle it properly, so max timeout might be good? trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java <http://review.cloudera.org/r/1298/#comment6533> whitespace trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java <http://review.cloudera.org/r/1298/#comment6532> maybe this should be warn. i think i'd want to see it and also logging of stack trace (i don't see logging of it elsewhere) - Jonathan On 2010-12-15 16:14:27, stack wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.cloudera.org/r/1298/ > ----------------------------------------------------------- > > (Updated 2010-12-15 16:14:27) > > > Review request for hbase and Jonathan Gray. > > > Summary > ------- > > M > src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java > Removed stale comments and TODOs. > > Added a 'version' datamenber, the znode edit version which we keep across > open process. > > Refactored the setting of OPENING out into a method that is used in multiple > places > now rather than repeat code. Did this in new tickleOpening method. > > Added new PostOpenDeployTasksThread which we run to do the > postOpenDeployTasks. > While its running we update OPENING state if its running a while. > > > This addresses bug hbase-3362. > http://issues.apache.org/jira/browse/hbase-3362 > > > Diffs > ----- > > > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java > 1049707 > > Diff: http://review.cloudera.org/r/1298/diff > > > Testing > ------- > > Ran it on my cluster. Seems to work as the old code did. > > > Thanks, > > stack > >
