You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by st...@apache.org on 2012/01/07 23:16:12 UTC

svn commit: r1228740 - in /hbase/trunk/src: main/java/org/apache/hadoop/hbase/ipc/ main/java/org/apache/hadoop/hbase/master/ main/java/org/apache/hadoop/hbase/master/handler/ main/java/org/apache/hadoop/hbase/monitoring/ test/java/org/apache/hadoop/hba...

Author: stack
Date: Sat Jan  7 22:16:11 2012
New Revision: 1228740

URL: http://svn.apache.org/viewvc?rev=1228740&view=rev
Log:
HBASE-5141 Memory leak in MonitoredRPCHandlerImpl

Modified:
    hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
    hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
    hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ClosedRegionHandler.java
    hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java
    hbase/trunk/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java
    hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java

Modified: hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
URL: http://svn.apache.org/viewvc/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java?rev=1228740&r1=1228739&r2=1228740&view=diff
==============================================================================
--- hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java (original)
+++ hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java Sat Jan  7 22:16:11 2012
@@ -1343,6 +1343,7 @@ public abstract class HBaseServer implem
                 errorClass, error);
           }
           call.sendResponseIfReady();
+          status.markComplete("Sent response");
         } catch (InterruptedException e) {
           if (running) {                          // unexpected -- log it
             LOG.info(getName() + " caught: " +

Modified: hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
URL: http://svn.apache.org/viewvc/hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java?rev=1228740&r1=1228739&r2=1228740&view=diff
==============================================================================
--- hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java (original)
+++ hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java Sat Jan  7 22:16:11 2012
@@ -711,13 +711,13 @@ public class AssignmentManager extends Z
           // Handle CLOSED by assigning elsewhere or stopping if a disable
           // If we got here all is good.  Need to update RegionState -- else
           // what follows will fail because not in expected state.
-          regionState.update(RegionState.State.CLOSED,
-              data.getStamp(), data.getOrigin());
-        removeClosedRegion(regionState.getRegion());
-          this.executorService.submit(new ClosedRegionHandler(master,
-            this, regionState.getRegion()));
+          regionState.update(RegionState.State.CLOSED, data.getStamp(),
+            data.getOrigin());
+          removeClosedRegion(regionState.getRegion());
+          this.executorService.submit(new ClosedRegionHandler(master, this,
+            regionState.getRegion()));
           break;
-          
+
         case RS_ZK_REGION_FAILED_OPEN:
           if (regionState == null ||
               (!regionState.isPendingOpen() && !regionState.isOpening())) {
@@ -1193,8 +1193,7 @@ public class AssignmentManager extends Z
         region.getRegionNameAsString());
       return;
     }
-    RegionState state = addToRegionsInTransition(region,
-        hijack);
+    RegionState state = addToRegionsInTransition(region, hijack);
     synchronized (state) {
       assign(region, state, setOfflineInZK, forceNewPlan, hijack);
     }
@@ -1393,9 +1392,8 @@ public class AssignmentManager extends Z
       this.regionsInTransition.put(encodedName, state);
     } else {
       // If we are reassigning the node do not force in-memory state to OFFLINE.
-      // Based on the znode state we will decide if to change
-      // in-memory state to OFFLINE or not. It will
-      // be done before setting the znode to OFFLINE state.
+      // Based on the znode state we will decide if to change in-memory state to
+      // OFFLINE or not. It will be done before setting znode to OFFLINE state.
       if (!hijack) {
         LOG.debug("Forcing OFFLINE; was=" + state);
         state.update(RegionState.State.OFFLINE);
@@ -1419,8 +1417,7 @@ public class AssignmentManager extends Z
       if (setOfflineInZK) {
         // get the version of the znode after setting it to OFFLINE.
         // versionOfOfflineNode will be -1 if the znode was not set to OFFLINE
-        versionOfOfflineNode = setOfflineInZooKeeper(state,
-            hijack);
+        versionOfOfflineNode = setOfflineInZooKeeper(state, hijack);
         if(versionOfOfflineNode != -1){
           if (isDisabledorDisablingRegionInRIT(region)) {
             return;
@@ -1529,8 +1526,7 @@ public class AssignmentManager extends Z
    * @return the version of the offline node if setting of the OFFLINE node was
    *         successful, -1 otherwise.
    */
-  int setOfflineInZooKeeper(final RegionState state,
-      boolean hijack) {
+  int setOfflineInZooKeeper(final RegionState state, boolean hijack) {
     // In case of reassignment the current state in memory need not be
     // OFFLINE. 
     if (!hijack && !state.isClosed() && !state.isOffline()) {
@@ -1743,7 +1739,7 @@ public class AssignmentManager extends Z
       region.getRegionNameAsString() + " (offlining)");
     synchronized (this.regions) {
       // Check if this region is currently assigned
-      if (!regions.containsKey(region)) {
+      if (!this.regions.containsKey(region)) {
         LOG.debug("Attempted to unassign region " +
           region.getRegionNameAsString() + " but it is not " +
           "currently assigned anywhere");
@@ -2718,6 +2714,7 @@ public class AssignmentManager extends Z
 
     return matchAM;
   }
+
   /**
    * Process shutdown server removing any assignments.
    * @param sn Server that went down.

Modified: hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ClosedRegionHandler.java
URL: http://svn.apache.org/viewvc/hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ClosedRegionHandler.java?rev=1228740&r1=1228739&r2=1228740&view=diff
==============================================================================
--- hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ClosedRegionHandler.java (original)
+++ hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ClosedRegionHandler.java Sat Jan  7 22:16:11 2012
@@ -98,6 +98,7 @@ public class ClosedRegionHandler extends
     }
     // ZK Node is in CLOSED state, assign it.
     assignmentManager.setOffline(regionInfo);
+    // This below has to do w/ online enable/disable of a table
     assignmentManager.removeClosedRegion(regionInfo);
     assignmentManager.assign(regionInfo, true);
   }

Modified: hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java
URL: http://svn.apache.org/viewvc/hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java?rev=1228740&r1=1228739&r2=1228740&view=diff
==============================================================================
--- hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java (original)
+++ hbase/trunk/src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java Sat Jan  7 22:16:11 2012
@@ -53,7 +53,6 @@ public class ServerShutdownHandler exten
   private final ServerName serverName;
   private final MasterServices services;
   private final DeadServer deadServers;
-  private final boolean shouldSplitHlog; // whether to split HLog or not
 
   public ServerShutdownHandler(final Server server, final MasterServices services,
       final DeadServer deadServers, final ServerName serverName,
@@ -73,7 +72,6 @@ public class ServerShutdownHandler exten
     if (!this.deadServers.contains(this.serverName)) {
       LOG.warn(this.serverName + " is NOT in deadservers; it should be!");
     }
-    this.shouldSplitHlog = shouldSplitHlog;
   }
 
   @Override
@@ -186,7 +184,7 @@ public class ServerShutdownHandler exten
         LOG.info("Server " + serverName +
             " was carrying ROOT. Trying to assign.");
         this.services.getAssignmentManager().
-        regionOffline(HRegionInfo.ROOT_REGIONINFO);
+          regionOffline(HRegionInfo.ROOT_REGIONINFO);
         verifyAndAssignRootWithRetries();
       }
 
@@ -195,7 +193,7 @@ public class ServerShutdownHandler exten
         LOG.info("Server " + serverName +
           " was carrying META. Trying to assign.");
         this.services.getAssignmentManager().
-        regionOffline(HRegionInfo.FIRST_META_REGIONINFO);
+          regionOffline(HRegionInfo.FIRST_META_REGIONINFO);
         this.services.getAssignmentManager().assignMeta();
       }
 
@@ -228,9 +226,8 @@ public class ServerShutdownHandler exten
       // OFFLINE? -- and then others after like CLOSING that depend on log
       // splitting.
       List<RegionState> regionsInTransition =
-        this.services.getAssignmentManager()
-        .processServerShutdown(this.serverName);
-
+        this.services.getAssignmentManager().
+          processServerShutdown(this.serverName);
 
       // Wait on meta to come online; we need it to progress.
       // TODO: Best way to hold strictly here?  We should build this retry logic
@@ -267,8 +264,8 @@ public class ServerShutdownHandler exten
       for (RegionState rit : regionsInTransition) {
         if (!rit.isClosing() && !rit.isPendingClose()) {
           LOG.debug("Removed " + rit.getRegion().getRegionNameAsString() +
-          " from list of regions to assign because in RIT" + " region state: "
-          + rit.getState());
+          " from list of regions to assign because in RIT; region state: " +
+          rit.getState());
           if (hris != null) hris.remove(rit.getRegion());
         }
       }

Modified: hbase/trunk/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java
URL: http://svn.apache.org/viewvc/hbase/trunk/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java?rev=1228740&r1=1228739&r2=1228740&view=diff
==============================================================================
--- hbase/trunk/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java (original)
+++ hbase/trunk/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java Sat Jan  7 22:16:11 2012
@@ -217,6 +217,13 @@ public class MonitoredRPCHandlerImpl ext
     this.remotePort = remotePort;
   }
 
+  @Override
+  public void markComplete(String status) {
+    super.markComplete(status);
+    this.params = null;
+    this.packet = null;
+  }
+
   public synchronized Map<String, Object> toMap() {
     // only include RPC info if the Handler is actively servicing an RPC call
     Map<String, Object> map = super.toMap();

Modified: hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java
URL: http://svn.apache.org/viewvc/hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java?rev=1228740&r1=1228739&r2=1228740&view=diff
==============================================================================
--- hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java (original)
+++ hbase/trunk/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java Sat Jan  7 22:16:11 2012
@@ -53,6 +53,8 @@ public class TestAssignmentManager {
   private static final HBaseTestingUtility HTU = new HBaseTestingUtility();
   private static final ServerName RANDOM_SERVERNAME =
     new ServerName("example.org", 1234, 5678);
+  private static final ServerName SRC_SERVERNAME =
+    new ServerName("example.org", 5678, 5678);
   private Server server;
   private ServerManager serverManager;
   private CatalogTracker ct;
@@ -95,6 +97,25 @@ public class TestAssignmentManager {
   }
 
   @Test
+  public void testBalanceAndShutdownServerAtSameTime()
+  throws IOException, KeeperException {
+    // Region to use in test.
+    final HRegionInfo hri = HRegionInfo.FIRST_META_REGIONINFO;
+    // Amend ServerManager mock so that when we do send close of our region on
+    // RANDOM_SERVERNAME, it will return true rather than default null.
+    Mockito.when(this.serverManager.sendRegionClose(RANDOM_SERVERNAME, hri)).
+      thenReturn(true);
+    // Create an AM.
+    AssignmentManager am =
+      new AssignmentManager(this.server, this.serverManager, this.ct, this.executor);
+    // Call the balance function
+    RegionPlan plan = new RegionPlan(hri, SRC_SERVERNAME, RANDOM_SERVERNAME);
+    am.balance(plan);
+    // This delete will fail if the previous unassign did wrong thing.
+    ZKAssign.deleteClosingNode(this.watcher, hri);
+  }
+
+  @Test
   public void testUnassignWithSplitAtSameTime() throws KeeperException, IOException {
     // Region to use in test.
     final HRegionInfo hri = HRegionInfo.FIRST_META_REGIONINFO;
@@ -182,4 +203,4 @@ public class TestAssignmentManager {
   @org.junit.Rule
   public org.apache.hadoop.hbase.ResourceCheckerJUnitRule cu =
     new org.apache.hadoop.hbase.ResourceCheckerJUnitRule();
-}
+}
\ No newline at end of file