Return-Path: X-Original-To: apmail-hbase-commits-archive@www.apache.org Delivered-To: apmail-hbase-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id A40189EBB for ; Sat, 16 Jun 2012 03:13:26 +0000 (UTC) Received: (qmail 82992 invoked by uid 500); 16 Jun 2012 03:13:26 -0000 Delivered-To: apmail-hbase-commits-archive@hbase.apache.org Received: (qmail 82840 invoked by uid 500); 16 Jun 2012 03:13:26 -0000 Mailing-List: contact commits-help@hbase.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@hbase.apache.org Delivered-To: mailing list commits@hbase.apache.org Received: (qmail 82830 invoked by uid 99); 16 Jun 2012 03:13:25 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 16 Jun 2012 03:13:25 +0000 X-ASF-Spam-Status: No, hits=-2000.0 required=5.0 tests=ALL_TRUSTED X-Spam-Check-By: apache.org Received: from [140.211.11.4] (HELO eris.apache.org) (140.211.11.4) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 16 Jun 2012 03:13:24 +0000 Received: from eris.apache.org (localhost [127.0.0.1]) by eris.apache.org (Postfix) with ESMTP id 4082B2388A32 for ; Sat, 16 Jun 2012 03:13:04 +0000 (UTC) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: svn commit: r1350850 - /hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java Date: Sat, 16 Jun 2012 03:13:04 -0000 To: commits@hbase.apache.org From: mbautin@apache.org X-Mailer: svnmailer-1.0.8-patched Message-Id: <20120616031304.4082B2388A32@eris.apache.org> X-Virus-Checked: Checked by ClamAV on apache.org Author: mbautin Date: Sat Jun 16 03:13:03 2012 New Revision: 1350850 URL: http://svn.apache.org/viewvc?rev=1350850&view=rev Log: [master] remove serversToServerInfo shutdown Author: pkhemani Summary: serversToServersInfo is a concurrent-hash-map. Access to it doesn't have to be synchronized. I have removed all sync calls to serversToServerInfo except when master is shutting down and waiting for serversToServerInfo to be cleaned up. (I will mark that place) Test Plan: will retest rolling restart. but since this change relaxes locking, correctness has to be established by code review Reviewers: liyintang, kranganathan, kannan, aaiyer Reviewed By: kannan CC: hbase-eng@, mbautin Differential Revision: https://phabricator.fb.com/D495995 Task ID: 1120663 Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java?rev=1350850&r1=1350849&r2=1350850&view=diff ============================================================================== --- hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java (original) +++ hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java Sat Jun 16 03:13:03 2012 @@ -363,11 +363,9 @@ public class ServerManager { LOG.debug("region server race condition detected: " + info.getServerName()); } - - synchronized (this.serversToServerInfo) { - processServerInfoOnShutdown(info.getServerName(), true); - notifyServers(); - } + + processServerInfoOnShutdown(info.getServerName(), true); + notifyServers(); return new HMsg[] {HMsg.REGIONSERVER_STOP}; } else { @@ -420,48 +418,46 @@ public class ServerManager { * @param msgs */ private void processRegionServerExit(HServerInfo serverInfo, HMsg[] msgs) { - synchronized (this.serversToServerInfo) { - // This method removes ROOT/META from the list and marks them to be - // reassigned in addition to other housework. - if (processServerInfoOnShutdown(serverInfo.getServerName(), false)) { - // Only process the exit message if the server still has registered info. - // Otherwise we could end up processing the server exit twice. - LOG.info("Region server " + serverInfo.getServerName() + + // This method removes ROOT/META from the list and marks them to be + // reassigned in addition to other housework. + if (processServerInfoOnShutdown(serverInfo.getServerName(), false)) { + // Only process the exit message if the server still has registered info. + // Otherwise we could end up processing the server exit twice. + LOG.info("Region server " + serverInfo.getServerName() + ": MSG_REPORT_EXITING"); - // Get all the regions the server was serving reassigned - // (if we are not shutting down). - if (!master.isClosed()) { - for (int i = 1; i < msgs.length; i++) { - LOG.info("Processing " + msgs[i] + " from " + + // Get all the regions the server was serving reassigned + // (if we are not shutting down). + if (!master.isClosed()) { + for (int i = 1; i < msgs.length; i++) { + LOG.info("Processing " + msgs[i] + " from " + serverInfo.getServerName()); - assert msgs[i].getType() == HMsg.Type.MSG_REGION_CLOSE; - HRegionInfo info = msgs[i].getRegionInfo(); - // Meta/root region offlining is handed in removeServerInfo above. - if (!info.isMetaRegion()) { - synchronized (master.getRegionManager()) { - if (!master.getRegionManager().isOfflined(info.getRegionNameAsString())) { - master.getRegionManager().setUnassigned(info, true); - } else { - master.getRegionManager().removeRegion(info); - } + assert msgs[i].getType() == HMsg.Type.MSG_REGION_CLOSE; + HRegionInfo info = msgs[i].getRegionInfo(); + // Meta/root region offlining is handed in removeServerInfo above. + if (!info.isMetaRegion()) { + synchronized (master.getRegionManager()) { + if (!master.getRegionManager().isOfflined(info.getRegionNameAsString())) { + master.getRegionManager().setUnassigned(info, true); + } else { + master.getRegionManager().removeRegion(info); } } } } - // There should not be any regions in transition for this server - the - // server should finish transitions itself before closing - Map inTransition = master.getRegionManager() - .getRegionsInTransitionOnServer(serverInfo.getServerName()); - for (Map.Entry entry : inTransition.entrySet()) { - LOG.warn("Region server " + serverInfo.getServerName() - + " shut down with region " + entry.getKey() + " in transition " - + "state " + entry.getValue()); - master.getRegionManager().setUnassigned(entry.getValue().getRegionInfo(), - true); - } - LOG.info("Removing server's info " + serverInfo.getServerName()); - this.serversToServerInfo.remove(serverInfo.getServerName()); } + // There should not be any regions in transition for this server - the + // server should finish transitions itself before closing + Map inTransition = master.getRegionManager() + .getRegionsInTransitionOnServer(serverInfo.getServerName()); + for (Map.Entry entry : inTransition.entrySet()) { + LOG.warn("Region server " + serverInfo.getServerName() + + " shut down with region " + entry.getKey() + " in transition " + + "state " + entry.getValue()); + master.getRegionManager().setUnassigned(entry.getValue().getRegionInfo(), + true); + } + LOG.info("Removing server's info " + serverInfo.getServerName()); + this.serversToServerInfo.remove(serverInfo.getServerName()); } } @@ -921,9 +917,7 @@ public class ServerManager { * @return Read-only map of servers to serverinfo. */ public Map getServersToServerInfo() { - synchronized (this.serversToServerInfo) { - return Collections.unmodifiableMap(this.serversToServerInfo); - } + return Collections.unmodifiableMap(this.serversToServerInfo); } /** @@ -932,11 +926,9 @@ public class ServerManager { * if nothing found. */ public HServerInfo getHServerInfo(final HServerAddress hsa) { - synchronized(this.serversToServerInfo) { - // TODO: This is primitive. Do a better search. - for (Map.Entry e: this.serversToServerInfo.entrySet()) { - if (e.getValue().getServerAddress().equals(hsa)) return e.getValue(); - } + // TODO: This is primitive. Do a better search. + for (Map.Entry e: this.serversToServerInfo.entrySet()) { + if (e.getValue().getServerAddress().equals(hsa)) return e.getValue(); } return null; } @@ -945,9 +937,7 @@ public class ServerManager { * @return Read-only map of servers to load. */ public Map getServersToLoad() { - synchronized (this.serversToLoad) { - return Collections.unmodifiableMap(serversToLoad); - } + return Collections.unmodifiableMap(serversToLoad); } /** @@ -960,16 +950,16 @@ public class ServerManager { } /** + * There is no way to guarantee that the returned servers are really online + * * @return the list of the HServerAddress for all the online region servers. */ public List getOnlineRegionServerList() { - synchronized(this.serversToServerInfo) { - List serverList = new ArrayList(); - for (HServerInfo serverInfo: this.serversToServerInfo.values()) { - serverList.add(serverInfo.getServerAddress()); - } - return serverList; + List serverList = new ArrayList(); + for (HServerInfo serverInfo: this.serversToServerInfo.values()) { + serverList.add(serverInfo.getServerAddress()); } + return serverList; } /**