From daff5e8d96bfbb62ae63a3e8190b1bd29999986b Mon Sep 17 00:00:00 2001 From: stack Date: Tue, 25 Nov 2014 17:22:09 -0800 Subject: [PATCH] HBASE-12518 Task 4 polish. Remove CM#{get,delete}Connection Remove deleteConnection from everywhere except the two tests that ensure it does the right thing and from HCM and CM themselves. Undoes deleteConnection from tests and from the web ui --- .../hbase/tmpl/master/MasterStatusTmpl.jamon | 23 ++++++----- .../hbase/master/MasterStatusServlet.java | 4 +- .../hbase/regionserver/RSStatusServlet.java | 8 ++-- .../hbase-webapps/master/snapshot.jsp | 39 ++++++++++--------- .../resources/hbase-webapps/master/table.jsp | 38 +++++++++--------- .../hbase/TestMetaTableAccessorNoCluster.java | 6 +-- .../hadoop/hbase/TestMetaTableLocator.java | 24 +++++------- .../hbase/client/TestFromClientSide.java | 5 +++ .../apache/hadoop/hbase/client/TestHCM.java | 2 + .../hbase/master/TestCatalogJanitor.java | 3 -- .../hbase/master/TestMasterStatusServlet.java | 23 +++++------ 11 files changed, 86 insertions(+), 89 deletions(-) diff --git a/hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon b/hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon index 2f0451c0f57..97921263c9f 100644 --- a/hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon +++ b/hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon @@ -18,7 +18,6 @@ limitations under the License. <%args> HMaster master; -HBaseAdmin admin; Map frags = null; ServerName metaLocation = null; List servers = null; @@ -42,7 +41,7 @@ org.apache.hadoop.hbase.HConstants; org.apache.hadoop.hbase.NamespaceDescriptor; org.apache.hadoop.hbase.ServerLoad; org.apache.hadoop.hbase.ServerName; -org.apache.hadoop.hbase.client.HBaseAdmin; +org.apache.hadoop.hbase.client.Admin; org.apache.hadoop.hbase.client.HConnectionManager; org.apache.hadoop.hbase.HTableDescriptor; org.apache.hadoop.hbase.HBaseConfiguration; @@ -307,8 +306,11 @@ AssignmentManager assignmentManager = master.getAssignmentManager(); <%def catalogTables> <%java> - HTableDescriptor[] sysTables = master.isInitialized() ? admin.listTableDescriptorsByNamespace( - NamespaceDescriptor.SYSTEM_NAMESPACE_NAME_STR) : null; + HTableDescriptor[] sysTables = null; + try (Admin admin = master.getConnection().getAdmin()) { + sysTables = master.isInitialized() ? admin.listTableDescriptorsByNamespace( + NamespaceDescriptor.SYSTEM_NAMESPACE_NAME_STR) : null; + } <%if (sysTables != null && sysTables.length > 0)%> @@ -347,7 +349,10 @@ AssignmentManager assignmentManager = master.getAssignmentManager(); <%def userTables> <%java> - HTableDescriptor[] tables = master.isInitialized() ? admin.listTables() : null; + HTableDescriptor[] tables = null; + try (Admin admin = master.getConnection().getAdmin()) { + tables = master.isInitialized() ? admin.listTables() : null; + } <%if (tables != null && tables.length > 0)%>
@@ -379,7 +384,10 @@ AssignmentManager assignmentManager = master.getAssignmentManager(); <%def userSnapshots> <%java> - List snapshots = master.isInitialized() ? admin.listSnapshots() : null; + List snapshots = null; + try (Admin admin = master.getConnection().getAdmin()) { + snapshots = master.isInitialized() ? admin.listSnapshots() : null; + } <%if (snapshots != null && snapshots.size() > 0)%>
@@ -436,7 +444,4 @@ AssignmentManager assignmentManager = master.getAssignmentManager();
-<%java> - HConnectionManager.deleteConnection(admin.getConfiguration()); - diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterStatusServlet.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterStatusServlet.java index bc9d1db8099..c56df54b4f3 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterStatusServlet.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterStatusServlet.java @@ -53,7 +53,6 @@ public class MasterStatusServlet extends HttpServlet { response.setContentType("text/html"); Configuration conf = master.getConfiguration(); - HBaseAdmin admin = new HBaseAdmin(conf); Map frags = getFragmentationInfo(master, conf); ServerName metaLocation = null; @@ -80,8 +79,7 @@ public class MasterStatusServlet extends HttpServlet { tmpl.setFilter(request.getParameter("filter")); if (request.getParameter("format") != null) tmpl.setFormat(request.getParameter("format")); - tmpl.render(response.getWriter(), - master, admin); + tmpl.render(response.getWriter(), master); } private ServerName getMetaLocationOrNull(HMaster master) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSStatusServlet.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSStatusServlet.java index 6f5c03ffcac..f1f5892a97c 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSStatusServlet.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSStatusServlet.java @@ -34,10 +34,8 @@ public class RSStatusServlet extends HttpServlet { @Override protected void doGet(HttpServletRequest req, HttpServletResponse resp) - throws ServletException, IOException - { - HRegionServer hrs = (HRegionServer)getServletContext().getAttribute( - HRegionServer.REGIONSERVER); + throws ServletException, IOException { + HRegionServer hrs = (HRegionServer)getServletContext().getAttribute(HRegionServer.REGIONSERVER); assert hrs != null : "No RS in context!"; resp.setContentType("text/html"); @@ -59,4 +57,4 @@ public class RSStatusServlet extends HttpServlet { tmpl.setBcv(req.getParameter("bcv")); tmpl.render(resp.getWriter(), hrs); } -} +} \ No newline at end of file diff --git a/hbase-server/src/main/resources/hbase-webapps/master/snapshot.jsp b/hbase-server/src/main/resources/hbase-webapps/master/snapshot.jsp index 31fab53ebe1..831835e8d10 100644 --- a/hbase-server/src/main/resources/hbase-webapps/master/snapshot.jsp +++ b/hbase-server/src/main/resources/hbase-webapps/master/snapshot.jsp @@ -20,7 +20,7 @@ <%@ page contentType="text/html;charset=UTF-8" import="java.util.Date" import="org.apache.hadoop.conf.Configuration" - import="org.apache.hadoop.hbase.client.HBaseAdmin" + import="org.apache.hadoop.hbase.client.Admin" import="org.apache.hadoop.hbase.client.HConnectionManager" import="org.apache.hadoop.hbase.master.HMaster" import="org.apache.hadoop.hbase.snapshot.SnapshotInfo" @@ -31,18 +31,19 @@ <% HMaster master = (HMaster)getServletContext().getAttribute(HMaster.MASTER); Configuration conf = master.getConfiguration(); - HBaseAdmin hbadmin = new HBaseAdmin(conf); boolean readOnly = conf.getBoolean("hbase.master.ui.readonly", false); String snapshotName = request.getParameter("name"); SnapshotDescription snapshot = null; SnapshotInfo.SnapshotStats stats = null; TableName snapshotTable = null; - for (SnapshotDescription snapshotDesc: hbadmin.listSnapshots()) { - if (snapshotName.equals(snapshotDesc.getName())) { - snapshot = snapshotDesc; - stats = SnapshotInfo.getSnapshotStats(conf, snapshot); - snapshotTable = TableName.valueOf(snapshot.getTable()); - break; + try (Admin admin = master.getConnection().getAdmin()) { + for (SnapshotDescription snapshotDesc: admin.listSnapshots()) { + if (snapshotName.equals(snapshotDesc.getName())) { + snapshot = snapshotDesc; + stats = SnapshotInfo.getSnapshotStats(conf, snapshot); + snapshotTable = TableName.valueOf(snapshot.getTable()); + break; + } } } @@ -112,15 +113,17 @@

<% - if (action.equals("restore")) { - hbadmin.restoreSnapshot(snapshotName); - %> Restore Snapshot request accepted. <% - } else if (action.equals("clone")) { - if (cloneName != null && cloneName.length() > 0) { - hbadmin.cloneSnapshot(snapshotName, cloneName); - %> Clone from Snapshot request accepted. <% - } else { - %> Clone from Snapshot request failed, No table name specified. <% + try (Admin admin = master.getConnection().getAdmin()) { + if (action.equals("restore")) { + admin.restoreSnapshot(snapshotName); + %> Restore Snapshot request accepted. <% + } else if (action.equals("clone")) { + if (cloneName != null && cloneName.length() > 0) { + admin.cloneSnapshot(snapshotName, TableName.valueOf(cloneName)); + %> Clone from Snapshot request accepted. <% + } else { + %> Clone from Snapshot request failed, No table name specified. <% + } } } %> @@ -189,8 +192,6 @@ <% } %> <% } // end else - -HConnectionManager.deleteConnection(hbadmin.getConfiguration()); %> diff --git a/hbase-server/src/main/resources/hbase-webapps/master/table.jsp b/hbase-server/src/main/resources/hbase-webapps/master/table.jsp index 2d576d4caae..cd21fefe2ae 100644 --- a/hbase-server/src/main/resources/hbase-webapps/master/table.jsp +++ b/hbase-server/src/main/resources/hbase-webapps/master/table.jsp @@ -23,7 +23,7 @@ import="java.util.Map" import="org.apache.hadoop.conf.Configuration" import="org.apache.hadoop.hbase.client.HTable" - import="org.apache.hadoop.hbase.client.HBaseAdmin" + import="org.apache.hadoop.hbase.client.Admin" import="org.apache.hadoop.hbase.client.HConnectionManager" import="org.apache.hadoop.hbase.HRegionInfo" import="org.apache.hadoop.hbase.ServerName" @@ -41,7 +41,6 @@ <% HMaster master = (HMaster)getServletContext().getAttribute(HMaster.MASTER); Configuration conf = master.getConfiguration(); - HBaseAdmin hbadmin = new HBaseAdmin(conf); MetaTableLocator metaTableLocator = new MetaTableLocator(); String fqtn = request.getParameter("name"); HTable table = new HTable(conf, fqtn); @@ -125,21 +124,23 @@

<% - if (action.equals("split")) { - if (key != null && key.length() > 0) { - hbadmin.split(key); - } else { - hbadmin.split(fqtn); - } + try (Admin admin = master.getConnection().getAdmin()) { + if (action.equals("split")) { + if (key != null && key.length() > 0) { + admin.splitRegion(Bytes.toBytes(key)); + } else { + admin.split(TableName.valueOf(fqtn)); + } %> Split request accepted. <% - } else if (action.equals("compact")) { - if (key != null && key.length() > 0) { - hbadmin.compact(key); - } else { - hbadmin.compact(fqtn); - } + } else if (action.equals("compact")) { + if (key != null && key.length() > 0) { + admin.compactRegion(Bytes.toBytes(key)); + } else { + admin.compact(TableName.valueOf(fqtn)); + } %> Compact request accepted. <% + } } %>

Go Back, or wait for the redirect. @@ -220,6 +221,7 @@ <% } %> <%} else { + Admin admin = master.getConnection().getAdmin(); try { %>

Table Attributes

@@ -230,7 +232,7 @@ - + @@ -238,7 +240,7 @@
Enabled<%= hbadmin.isTableEnabled(table.getTableName()) %><%= admin.isTableEnabled(table.getName()) %> Is the table enabled
<% try { - CompactionState compactionState = hbadmin.getCompactionState(table.getTableName()); + CompactionState compactionState = admin.getCompactionState(table.getName()); %> <%= compactionState %> <% @@ -332,10 +334,10 @@ <% } } catch(Exception ex) { ex.printStackTrace(System.err); +} finally { + admin.close(); } } // end else - -HConnectionManager.deleteConnection(hbadmin.getConfiguration()); %> diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableAccessorNoCluster.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableAccessorNoCluster.java index b313d126c5a..2e9843fd3a3 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableAccessorNoCluster.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableAccessorNoCluster.java @@ -30,8 +30,6 @@ import java.util.NavigableMap; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.hbase.client.ClusterConnection; -import org.apache.hadoop.hbase.client.HConnection; -import org.apache.hadoop.hbase.client.HConnectionManager; import org.apache.hadoop.hbase.client.HConnectionTestingUtility; import org.apache.hadoop.hbase.client.Result; import org.apache.hadoop.hbase.ipc.PayloadCarryingRpcController; @@ -128,7 +126,7 @@ public class TestMetaTableAccessorNoCluster { // This is a servername we use in a few places below. ServerName sn = ServerName.valueOf("example.com", 1234, System.currentTimeMillis()); - HConnection connection; + ClusterConnection connection = null; try { // Mock an ClientProtocol. Our mock implementation will fail a few // times when we go to open a scanner. @@ -204,7 +202,7 @@ public class TestMetaTableAccessorNoCluster { Mockito.verify(implementation, Mockito.times(6)). scan((RpcController)Mockito.any(), (ScanRequest)Mockito.any()); } finally { - HConnectionManager.deleteConnection(UTIL.getConfiguration()); + if (connection != null && !connection.isClosed()) connection.close(); zkw.close(); } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableLocator.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableLocator.java index 8a439a82ad4..4512326a201 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableLocator.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableLocator.java @@ -29,8 +29,7 @@ import java.net.ConnectException; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; -import org.apache.hadoop.hbase.client.HConnection; -import org.apache.hadoop.hbase.client.HConnectionManager; +import org.apache.hadoop.hbase.client.ClusterConnection; import org.apache.hadoop.hbase.client.HConnectionTestingUtility; import org.apache.hadoop.hbase.ipc.ServerNotRunningYetException; import org.apache.hadoop.hbase.master.RegionState; @@ -101,9 +100,6 @@ public class TestMetaTableLocator { LOG.warn("Unable to delete hbase:meta location", e); } - // Clear out our doctored connection or could mess up subsequent tests. - HConnectionManager.deleteConnection(UTIL.getConfiguration()); - this.watcher.close(); } @@ -182,7 +178,8 @@ public class TestMetaTableLocator { // Mock an ClientProtocol. final ClientProtos.ClientService.BlockingInterface implementation = Mockito.mock(ClientProtos.ClientService.BlockingInterface.class); - HConnection connection = mockConnection(null, implementation); + + ClusterConnection connection = mockConnection(null, implementation); // If a 'get' is called on mocked interface, throw connection refused. Mockito.when(implementation.get((RpcController) Mockito.any(), (GetRequest) Mockito.any())). @@ -242,14 +239,14 @@ public class TestMetaTableLocator { @Test public void testVerifyMetaRegionLocationFails() throws IOException, InterruptedException, KeeperException, ServiceException { - HConnection connection = Mockito.mock(HConnection.class); + ClusterConnection connection = Mockito.mock(ClusterConnection.class); ServiceException connectException = new ServiceException(new ConnectException("Connection refused")); final AdminProtos.AdminService.BlockingInterface implementation = Mockito.mock(AdminProtos.AdminService.BlockingInterface.class); Mockito.when(implementation.getRegionInfo((RpcController)Mockito.any(), (GetRegionInfoRequest)Mockito.any())).thenThrow(connectException); - Mockito.when(connection.getAdmin(Mockito.any(ServerName.class), Mockito.anyBoolean())). + Mockito.when(connection.getAdmin(Mockito.any(ServerName.class))). thenReturn(implementation); ServerName sn = ServerName.valueOf("example.com", 1234, System.currentTimeMillis()); @@ -301,20 +298,17 @@ public class TestMetaTableLocator { * and that returns the passed {@link AdminProtos.AdminService.BlockingInterface} instance when * {@link HConnection#getAdmin(ServerName)} is called, returns the passed * {@link ClientProtos.ClientService.BlockingInterface} instance when - * {@link HConnection#getClient(ServerName)} is called (Be sure to call - * {@link HConnectionManager#deleteConnection(org.apache.hadoop.conf.Configuration)} - * when done with this mocked Connection. + * {@link HConnection#getClient(ServerName)} is called. * @throws IOException */ - private HConnection mockConnection(final AdminProtos.AdminService.BlockingInterface admin, + private ClusterConnection mockConnection(final AdminProtos.AdminService.BlockingInterface admin, final ClientProtos.ClientService.BlockingInterface client) throws IOException { - HConnection connection = + ClusterConnection connection = HConnectionTestingUtility.getMockedConnection(UTIL.getConfiguration()); Mockito.doNothing().when(connection).close(); // Make it so we return any old location when asked. - final HRegionLocation anyLocation = - new HRegionLocation(HRegionInfo.FIRST_META_REGIONINFO, SN); + final HRegionLocation anyLocation = new HRegionLocation(HRegionInfo.FIRST_META_REGIONINFO, SN); Mockito.when(connection.getRegionLocation((TableName) Mockito.any(), (byte[]) Mockito.any(), Mockito.anyBoolean())). thenReturn(anyLocation); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java index 1835e0ed9ca..aa9bcc6d44a 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java @@ -292,6 +292,11 @@ public class TestFromClientSide { table.close(); } + /** + * @deprecated Tests deprecated functionality. Remove when we are past 1.0. + * @throws Exception + */ + @Deprecated @Test public void testSharedZooKeeper() throws Exception { Configuration newConfig = new Configuration(TEST_UTIL.getConfiguration()); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestHCM.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestHCM.java index 58d7f2f4be9..57b9f6f2a8b 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestHCM.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestHCM.java @@ -874,7 +874,9 @@ public class TestHCM { * Makes sure that there is no leaking of * {@link ConnectionManager.HConnectionImplementation} in the {@link HConnectionManager} * class. + * @deprecated Tests deprecated functionality. Remove in 1.0. */ + @Deprecated @Test public void testConnectionUniqueness() throws Exception { int zkmaxconnections = TEST_UTIL.getConfiguration(). diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java index afbcfa14f2d..fe36d845885 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java @@ -195,9 +195,6 @@ public class TestCatalogJanitor { @Override public void stop(String why) { - if (this.connection != null) { - HConnectionManager.deleteConnection(this.connection.getConfiguration()); - } } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterStatusServlet.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterStatusServlet.java index fbc2c1ae6ac..ea72503b738 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterStatusServlet.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterStatusServlet.java @@ -52,7 +52,7 @@ import com.google.common.collect.Maps; */ @Category(MediumTests.class) public class TestMasterStatusServlet { - + private HMaster master; private Configuration conf; private HBaseAdmin admin; @@ -108,7 +108,7 @@ public class TestMasterStatusServlet { Mockito.doReturn(rms).when(master).getRegionServerMetrics(); // Mock admin - admin = Mockito.mock(HBaseAdmin.class); + admin = Mockito.mock(HBaseAdmin.class); } private void setupMockTables() throws IOException { @@ -118,27 +118,25 @@ public class TestMasterStatusServlet { }; Mockito.doReturn(tables).when(admin).listTables(); } - + @Test public void testStatusTemplateNoTables() throws IOException { - new MasterStatusTmpl().render(new StringWriter(), - master, admin); + new MasterStatusTmpl().render(new StringWriter(), master); } @Test public void testStatusTemplateMetaAvailable() throws IOException { setupMockTables(); - + new MasterStatusTmpl() .setMetaLocation(ServerName.valueOf("metaserver:123,12345")) - .render(new StringWriter(), - master, admin); + .render(new StringWriter(), master); } @Test public void testStatusTemplateWithServers() throws IOException { setupMockTables(); - + List servers = Lists.newArrayList( ServerName.valueOf("rootserver:123,12345"), ServerName.valueOf("metaserver:123,12345")); @@ -152,10 +150,9 @@ public class TestMasterStatusServlet { .setMetaLocation(ServerName.valueOf("metaserver:123,12345")) .setServers(servers) .setDeadServers(deadServers) - .render(new StringWriter(), - master, admin); + .render(new StringWriter(), master); } - + @Test public void testAssignmentManagerTruncatedList() throws IOException { AssignmentManager am = Mockito.mock(AssignmentManager.class); @@ -187,7 +184,7 @@ public class TestMasterStatusServlet { // Should always include META assertTrue(result.contains(HRegionInfo.FIRST_META_REGIONINFO.getEncodedName())); - + // Make sure we only see 50 of them Matcher matcher = Pattern.compile("CLOSING").matcher(result); int count = 0;