From 24b0a5a5e867168c39ef6bee6b9c5278be45f455 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 ++++++++----- .../hadoop/hbase/master/MasterStatusServlet.java | 4 +-- .../hadoop/hbase/regionserver/RSStatusServlet.java | 8 ++--- .../resources/hbase-webapps/master/snapshot.jsp | 39 +++++++++++----------- .../main/resources/hbase-webapps/master/table.jsp | 38 +++++++++++---------- .../hbase/TestMetaTableAccessorNoCluster.java | 6 ++-- .../apache/hadoop/hbase/TestMetaTableLocator.java | 24 +++++-------- .../hadoop/hbase/client/TestFromClientSide.java | 5 +++ .../org/apache/hadoop/hbase/client/TestHCM.java | 2 ++ .../hadoop/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 2f0451c..9792126 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 bc9d1db..c56df54 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 6f5c03f..f1f5892 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 31fab53..831835e 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 2b7829c..1f1871c 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 %> <% @@ -335,10 +337,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 62b4b9a..f70a0d7 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; @@ -130,7 +128,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. @@ -206,7 +204,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 05699e4..9943749 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; @@ -103,9 +102,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(); } @@ -184,7 +180,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())). @@ -244,14 +241,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()); @@ -303,20 +300,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 86ba189..e20e500 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 @@ -297,6 +297,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 e8f668b..665435a 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 @@ -875,7 +875,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 58afc12..cc501ed 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 @@ -199,9 +199,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 31d772a..b23ca78 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 @@ -54,7 +54,7 @@ import com.google.common.collect.Maps; */ @Category({MasterTests.class,MediumTests.class}) public class TestMasterStatusServlet { - + private HMaster master; private Configuration conf; private HBaseAdmin admin; @@ -110,7 +110,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 { @@ -120,27 +120,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")); @@ -154,10 +152,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); @@ -189,7 +186,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; -- 1.8.5.2 (Apple Git-48)