svn commit: r1182040 - in /hbase/branches/0.89/src: main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerFileSystemFailure.java

[ns...@apache.org]

Author: nspiegelberg
Date: Tue Oct 11 19:13:34 2011
New Revision: 1182040

URL: http://svn.apache.org/viewvc?rev=1182040&view=rev
Log:
Fixing TestRemoteTable and TestHRegionServerFileSystemFailure broken by D336813

Summary:
In D336813 I broke two unit tests, which was hard to notice because they failed
with time-outs. TestHRegionServerFileSystemFailure re-starts a master in the end
if it had crashed so that regionservers can shut down properly, but the
condition it uses to identify whether the master is still alive was no longer
valid, because there is now a thread object wrapping HMaster, and
master.isAlive() is meaningless. I replaced this with master.isClosed() and
removed hard-coded master class from the unit test.

TestRemoteTable started failing because of a bug in JVMClusterUtil.shutdown()
-- it did not call join() on master threads.

Also calling Thread.currentThread().interrupt() when handling
InterruptedException instead of simply ignoring it. This is recommended by Java
Concurrency in Practice as well as in other articles online as the proper way to
handle InterruptedException, but I must admit I don't fully understand why.

Test Plan: All 621 unit tests pass (compared to 614 without the patch).

Reviewers: pritam, kranganathan, pkhemani

Reviewed By: pritam

CC: hbase-eng@lists, pkhemani, pritam, mbautin

Differential Revision: 340934

Revert Plan: OK

Modified:
    hbase/branches/0.89/src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java
    hbase/branches/0.89/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java
    hbase/branches/0.89/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerFileSystemFailure.java

Modified: hbase/branches/0.89/src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.89/src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java?rev=1182040&r1=1182039&r2=1182040&view=diff
==============================================================================
--- hbase/branches/0.89/src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java (original)
+++ hbase/branches/0.89/src/main/java/org/apache/hadoop/hbase/util/JVMClusterUtil.java Tue Oct 11 19:13:34 2011
@@ -210,17 +210,38 @@ public class JVMClusterUtil {
         }
       }
     }
-    // regionServerThreads can never be null because they are initialized when
-    // the class is constructed.
-    for(Thread t: regionservers) {
-      if (t.isAlive()) {
-        try {
-          t.join();
-        } catch (InterruptedException e) {
-          // continue
+
+    boolean interrupted = false;
+    try {
+      // regionServerThreads can never be null because they are initialized when
+      // the class is constructed.
+      for (Thread t : regionservers) {
+        if (t.isAlive()) {
+          try {
+            t.join();
+          } catch (InterruptedException e) {
+            interrupted = true;
+          }
         }
       }
+
+      if (masters != null) {
+        for (JVMClusterUtil.MasterThread t : masters) {
+          while (t.isAlive()) {
+            try {
+              t.join();
+            } catch (InterruptedException e) {
+              interrupted = true;
+            }
+          }
+        }
+      }
+    } finally {
+      if (interrupted) {
+        Thread.currentThread().interrupt();
+      }
     }
+
     LOG.info("Shutdown of " +
         ((masters != null) ? masters.size() : "0") + " master(s) and " +
         ((regionservers != null) ? regionservers.size() : "0") +

Modified: hbase/branches/0.89/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.89/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java?rev=1182040&r1=1182039&r2=1182040&view=diff
==============================================================================
--- hbase/branches/0.89/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java (original)
+++ hbase/branches/0.89/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java Tue Oct 11 19:13:34 2011
@@ -288,6 +288,14 @@ public class MiniHBaseCluster {
   }
 
   /**
+   * Adds a new master to the cluster and starts the master thread. Useful if
+   * the existing master dies and a live master is needed for cleanup.
+   */
+  public void startNewMaster() throws IOException {
+    hbaseCluster.addMaster().start();
+  }
+
+  /**
    * Cause a region server to exit doing basic clean up only on its way out.
    * @param serverNumber  Used as index into a list.
    */

Modified: hbase/branches/0.89/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerFileSystemFailure.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.89/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerFileSystemFailure.java?rev=1182040&r1=1182039&r2=1182040&view=diff
==============================================================================
--- hbase/branches/0.89/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerFileSystemFailure.java (original)
+++ hbase/branches/0.89/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionServerFileSystemFailure.java Tue Oct 11 19:13:34 2011
@@ -96,9 +96,9 @@ public class TestHRegionServerFileSystem
     // Bring namenode, hbasemaster back up so we cleanup properly.
     TEST_UTIL.getDFSCluster().restartNameNode();
     HMaster master = TEST_UTIL.getMiniHBaseCluster().getMaster();
-    if (!master.isAlive()) {
-      master = HMaster.constructMaster(MiniHBaseCluster.MiniHBaseClusterMaster.class, conf);
-      master.start();
+    if (master.isClosed()) {
+      // Master died, we need to re-start it.
+      TEST_UTIL.getMiniHBaseCluster().startNewMaster();
     }
   }
 }