Review Board 1.7.22


HBASE-5134 Remove getRegionServerWithoutRetries and getRegionServerWithRetries from HConnection Interface

Review Request #3419 - Created Jan. 7, 2012 and updated

Michael Stack
HBASE-5134
Reviewers
hbase
hbase-git
Untangle HConnection.  Makes it so I can mock scanning which wasn't
possible when Callables had HConnections and HConnections had Callables.
I want this so we can write tests around some of the interesting master fails
we've seen of late; e.g. shutdown handler running at same time as a balance.

With this change I can do things like standup an AssignmentManager and
concurrently run a ServerShutdownHandler apart from their HMaster context;
I can fake out the ServerShutdownHandler feeding it whatever for metarows, etc.
Included are new tests that run an AssignmentManager#balance end-to-end and
that put up a ServerShutdownHandler and run a server shutdown processing.

M src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java
  Add a note that timeouts need fixup in here.
  Handle RetriesExhaustedException (Now we can mock at a lower-level
  than previous, below Callables where we used short-circuit them, we
  see Callable exceptions coming out in tests -- add handling).

M src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
  Callable retrying is done on the Callable now, not via HConnection.

A src/main/java/org/apache/hadoop/hbase/client/ConnectionUtils.java
  Move common connection utility here, shared by HConnection and Callable.
  
M src/main/java/org/apache/hadoop/hbase/client/HConnection.java
  (getRegionServerWithRetries, getRegionServerWithoutRetries): deprecated.
  They are done on the Callable itself itself now.
  (clearCaches): Added.

M src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
 (getPauseTime): Moved out to ConnectionUtils
 (getRegionServerWithRetries, getRegionServerWithoutRetries): deprecated.
 Changed body of methods to call new implementation.
 Removed dead code.  Added doc and comments on other stuff to move.
 (translateException): Moved out to ServerCallable where its used.

M src/main/java/org/apache/hadoop/hbase/client/HTable.java
  Callable retrying is done on the Callable now, not via HConnection.

M src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java
  Callable retrying is done on the Callable now, not via HConnection.

M src/main/java/org/apache/hadoop/hbase/client/ServerCallable.java
  (getConnection): Added
  (withRetries, withoutRetries): These replace old
   getRegionServerWithRetries, getRegionServerWithoutRetries that used to
   be done via HConnection.

M src/main/java/org/apache/hadoop/hbase/ipc/ExecRPCInvoker.java
  Callable retrying is done on the Callable now, not via HConnection.

M src/main/java/org/apache/hadoop/hbase/mapreduce/LoadIncrementalHFiles.java
  Callable retrying is done on the Callable now, not via HConnection.

M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
  Comment.

M src/main/java/org/apache/hadoop/hbase/master/handler/ClosedRegionHandler.java
  Comment.

M src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java
  Make use of a data member previous unused.

M src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
  Callables are in the mix now so stuff gets retried.  Deal with it.

M src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java
  Add a method that mocks out more methods in a mocked HConnection.

M src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java
  Comment.

M src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
  Add two tests, a testBalance and testServerShutdown

M src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java
  Use new utility.

M src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerBulkLoad.java
  Callables are in the mix now so stuff gets retried.  Deal with it.

 
Ship it!
Posted (Jan. 7, 2012, 1:17 a.m.)
This is so much cleaner than before.
+1 This does not belong here.
nice!
yep
yes
Why it this no longer needed?
  1. Yeah.  Its weird.  Eclipse flagged it.   And then I looked.  There is a local variable set to null and its never changed.  Then in this loop, we check if its non-null.  One day the machines will write all of the the programs for us. 
Posted (Jan. 7, 2012, 4:20 a.m.)

   

  
'Pass in a ServerCallable' should be removed.
  1. Yes.  This javadoc (and next method) is all wrong now its been moved and its context changed.  Good one.  Will fix in v2.
This line seems unnecessary.