diff --git a/CHANGES.txt b/CHANGES.txt index 83ddba004dd..f6ed01d929c 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -503,6 +503,9 @@ Release 0.21.0 - Unreleased HBASE-2943 major_compact (and other admin commands) broken for .META. HBASE-2643 Figure how to deal with eof splitting logs (Nicolas Spiegelberg via Stack) + HBASE-2925 LRU of HConnectionManager.HBASE_INSTANCES breaks if + HBaseConfiguration is changed + (Robert Mahfoud via Stack) IMPROVEMENTS HBASE-1760 Cleanup TODOs in HTable diff --git a/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java b/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java index 1e59eaa3b46..13900c3377b 100644 --- a/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java +++ b/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java @@ -19,7 +19,6 @@ */ package org.apache.hadoop.hbase; -import java.util.Iterator; import java.util.Map.Entry; import org.apache.commons.logging.Log; @@ -87,57 +86,4 @@ public class HBaseConfiguration extends Configuration { } return conf; } - - /** - * Returns the hash code value for this HBaseConfiguration. The hash code of a - * HBaseConfiguration is defined by the xor of the hash codes of its entries. - * - * @see Configuration#iterator() How the entries are obtained. - */ - @Override - @Deprecated - public int hashCode() { - return hashCode(this); - } - - /** - * Returns the hash code value for this HBaseConfiguration. The hash code of a - * Configuration is defined by the xor of the hash codes of its entries. - * - * @see Configuration#iterator() How the entries are obtained. - */ - public static int hashCode(Configuration conf) { - int hash = 0; - - Iterator> propertyIterator = conf.iterator(); - while (propertyIterator.hasNext()) { - hash ^= propertyIterator.next().hashCode(); - } - return hash; - } - - @Override - public boolean equals(Object obj) { - if (this == obj) - return true; - if (obj == null) - return false; - if (!(obj instanceof HBaseConfiguration)) - return false; - HBaseConfiguration otherConf = (HBaseConfiguration) obj; - if (size() != otherConf.size()) { - return false; - } - Iterator> propertyIterator = this.iterator(); - while (propertyIterator.hasNext()) { - Entry entry = propertyIterator.next(); - String key = entry.getKey(); - String value = entry.getValue(); - if (!value.equals(otherConf.getRaw(key))) { - return false; - } - } - - return true; - } } diff --git a/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java b/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java index ea1f998b833..d047acb2049 100644 --- a/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java +++ b/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java @@ -408,7 +408,7 @@ public class HBaseAdmin implements Abortable { } } // Delete cached information to prevent clients from using old locations - HConnectionManager.deleteConnectionInfo(conf, false); + HConnectionManager.deleteConnection(conf, false); LOG.info("Deleted " + Bytes.toString(tableName)); } diff --git a/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java b/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java index cc5ab7dc97f..87e17bceda8 100644 --- a/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java +++ b/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java @@ -45,7 +45,6 @@ import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.Abortable; import org.apache.hadoop.hbase.DoNotRetryIOException; -import org.apache.hadoop.hbase.HBaseConfiguration; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.HRegionInfo; import org.apache.hadoop.hbase.HRegionLocation; @@ -74,9 +73,7 @@ import org.apache.hadoop.ipc.RemoteException; import org.apache.zookeeper.KeeperException; /** - * A non-instantiable class that manages connections to multiple tables in - * multiple HBase instances. - * + * A non-instantiable class that manages connections to tables. * Used by {@link HTable} and {@link HBaseAdmin} */ @SuppressWarnings("serial") @@ -91,44 +88,43 @@ public class HConnectionManager { }); } + static final int MAX_CACHED_HBASE_INSTANCES = 31; + + // A LRU Map of Configuration hashcode -> TableServers. We set instances to 31. + // The zk default max connections to the ensemble from the one client is 30 so + // should run into zk issues before hit this value of 31. + private static final Map HBASE_INSTANCES = + new LinkedHashMap + ((int) (MAX_CACHED_HBASE_INSTANCES/0.75F)+1, 0.75F, true) { + @Override + protected boolean removeEldestEntry(Map.Entry eldest) { + return size() > MAX_CACHED_HBASE_INSTANCES; + } + }; + /* - * Not instantiable. + * Non-instantiable. */ protected HConnectionManager() { super(); } - private static final int MAX_CACHED_HBASE_INSTANCES=31; - // A LRU Map of master HBaseConfiguration -> connection information for that - // instance. The objects it contains are mutable and hence require - // synchronized access to them. We set instances to 31. The zk default max - // connections is 30 so should run into zk issues before hit this value of 31. - private static - final Map HBASE_INSTANCES = - new LinkedHashMap - ((int) (MAX_CACHED_HBASE_INSTANCES/0.75F)+1, 0.75F, true) { - @Override - protected boolean removeEldestEntry(Map.Entry eldest) { - return size() > MAX_CACHED_HBASE_INSTANCES; - } - }; - /** - * Get the connection object for the instance specified by the configuration - * If no current connection exists, create a new connection for that instance + * Get the connection that goes with the passed conf configuration. + * If no current connection exists, method creates a new connection for the + * passed conf instance. * @param conf configuration - * @return HConnection object for the instance specified by the configuration + * @return HConnection object for conf * @throws ZooKeeperConnectionException */ public static HConnection getConnection(Configuration conf) throws ZooKeeperConnectionException { TableServers connection; - Integer key = HBaseConfiguration.hashCode(conf); synchronized (HBASE_INSTANCES) { - connection = HBASE_INSTANCES.get(key); + connection = HBASE_INSTANCES.get(conf); if (connection == null) { connection = new TableServers(conf); - HBASE_INSTANCES.put(key, connection); + HBASE_INSTANCES.put(conf, connection); } } return connection; @@ -139,11 +135,9 @@ public class HConnectionManager { * @param conf configuration * @param stopProxy stop the proxy as well */ - public static void deleteConnectionInfo(Configuration conf, - boolean stopProxy) { + public static void deleteConnection(Configuration conf, boolean stopProxy) { synchronized (HBASE_INSTANCES) { - Integer key = HBaseConfiguration.hashCode(conf); - TableServers t = HBASE_INSTANCES.remove(key); + TableServers t = HBASE_INSTANCES.remove(conf); if (t != null) { t.close(stopProxy); } @@ -172,7 +166,8 @@ public class HConnectionManager { * @throws ZooKeeperConnectionException */ static int getCachedRegionCount(Configuration conf, - byte[] tableName) throws ZooKeeperConnectionException { + byte[] tableName) + throws ZooKeeperConnectionException { TableServers connection = (TableServers)getConnection(conf); return connection.getNumberOfCachedRegionLocations(tableName); } @@ -189,7 +184,7 @@ public class HConnectionManager { return connection.isRegionCached(tableName, row); } - /* Encapsulates finding the servers for an HBase instance */ + /* Encapsulates connection to zookeeper and regionservers.*/ static class TableServers implements ServerConnection, Abortable { static final Log LOG = LogFactory.getLog(TableServers.class); private final Class serverInterfaceClass; @@ -212,7 +207,7 @@ public class HConnectionManager { private final Object metaRegionLock = new Object(); private final Object userRegionLock = new Object(); - private volatile Configuration conf; + private final Configuration conf; // Known region HServerAddress.toString() -> HRegionInterface private final Map servers = diff --git a/src/main/java/org/apache/hadoop/hbase/client/HTable.java b/src/main/java/org/apache/hadoop/hbase/client/HTable.java index 45af92c5cbb..0eaced61048 100644 --- a/src/main/java/org/apache/hadoop/hbase/client/HTable.java +++ b/src/main/java/org/apache/hadoop/hbase/client/HTable.java @@ -60,12 +60,25 @@ import java.io.DataOutput; /** * Used to communicate with a single HBase table. * - * This class is not thread safe for writes. - * Gets, puts, and deletes take out a row lock for the duration - * of their operation. Scans (currently) do not respect - * row locking. + * This class is not thread safe for updates; the underlying write buffer can + * be corrupted if multiple threads contend over a single HTable instance. * - * See {@link HBaseAdmin} to create, drop, list, enable and disable tables. + *

Instances of HTable passed the same {@link Configuration} instance will + * share connections to master and the zookeeper ensemble as well as caches of + * region locations. This happens because they will all share the same + * {@link HConnection} instance (internally we keep a Map of {@link HConnection} + * instances keyed by {@link Configuration}). + * {@link HConnection} will read most of the + * configuration it needs from the passed {@link Configuration} on initial + * construction. Thereafter, for settings such as + * hbase.client.pause, hbase.client.retries.number, + * and hbase.client.rpc.maxattempts updating their values in the + * passed {@link Configuration} subsequent to {@link HConnection} construction + * will go unnoticed. To run with changed values, make a new + * {@link HTable} passing a new {@link Configuration} instance that has the + * new configuration. + * + * @see HBaseAdmin for create, drop, list, enable and disable of tables. */ public class HTable implements HTableInterface { private final HConnection connection; @@ -83,9 +96,13 @@ public class HTable implements HTableInterface { /** * Creates an object to access a HBase table. - * - * @param tableName Name of the table. + * Internally it creates a new instance of {@link Configuration} and a new + * client to zookeeper as well as other resources. It also comes up with + * a fresh view of the cluster and must do discovery from scratch of region + * locations; i.e. it will not make use of already-cached region locations if + * available. Use only when being quick and dirty. * @throws IOException if a remote or network exception occurs + * @see #HTable(Configuration, String) */ public HTable(final String tableName) throws IOException { @@ -94,9 +111,14 @@ public class HTable implements HTableInterface { /** * Creates an object to access a HBase table. - * + * Internally it creates a new instance of {@link Configuration} and a new + * client to zookeeper as well as other resources. It also comes up with + * a fresh view of the cluster and must do discovery from scratch of region + * locations; i.e. it will not make use of already-cached region locations if + * available. Use only when being quick and dirty. * @param tableName Name of the table. * @throws IOException if a remote or network exception occurs + * @see #HTable(Configuration, String) */ public HTable(final byte [] tableName) throws IOException { @@ -105,7 +127,10 @@ public class HTable implements HTableInterface { /** * Creates an object to access a HBase table. - * + * Shares zookeeper connection and other resources with other HTable instances + * created with the same conf instance. Uses already-populated + * region cache if one is available, populated by any other HTable instances + * sharing this conf instance. Recommended. * @param conf Configuration object to use. * @param tableName Name of the table. * @throws IOException if a remote or network exception occurs @@ -118,7 +143,10 @@ public class HTable implements HTableInterface { /** * Creates an object to access a HBase table. - * + * Shares zookeeper connection and other resources with other HTable instances + * created with the same conf instance. Uses already-populated + * region cache if one is available, populated by any other HTable instances + * sharing this conf instance. Recommended. * @param conf Configuration object to use. * @param tableName Name of the table. * @throws IOException if a remote or network exception occurs @@ -722,6 +750,10 @@ public class HTable implements HTableInterface { } } + /** + * Close down this HTable instance. + * Calls {@link #flushCommits()}. + */ public void close() throws IOException{ flushCommits(); } diff --git a/src/main/java/org/apache/hadoop/hbase/client/HTableFactory.java b/src/main/java/org/apache/hadoop/hbase/client/HTableFactory.java index 898701611f8..b7558968459 100644 --- a/src/main/java/org/apache/hadoop/hbase/client/HTableFactory.java +++ b/src/main/java/org/apache/hadoop/hbase/client/HTableFactory.java @@ -29,7 +29,6 @@ import java.io.IOException; * @since 0.21.0 */ public class HTableFactory implements HTableInterfaceFactory { - @Override public HTableInterface createHTableInterface(Configuration config, byte[] tableName) { @@ -47,9 +46,5 @@ public class HTableFactory implements HTableInterfaceFactory { } catch (IOException ioe) { throw new RuntimeException(ioe); } - } - - - -} +} \ No newline at end of file diff --git a/src/main/java/org/apache/hadoop/hbase/util/HMerge.java b/src/main/java/org/apache/hadoop/hbase/util/HMerge.java index c9190b14206..7a4c32349ed 100644 --- a/src/main/java/org/apache/hadoop/hbase/util/HMerge.java +++ b/src/main/java/org/apache/hadoop/hbase/util/HMerge.java @@ -106,7 +106,7 @@ class HMerge { HConnection connection = HConnectionManager.getConnection(conf); masterIsRunning = connection.isMasterRunning(); } - HConnectionManager.deleteConnectionInfo(conf, false); + HConnectionManager.deleteConnection(conf, false); if (Bytes.equals(tableName, HConstants.META_TABLE_NAME)) { if (masterIsRunning) { throw new IllegalStateException( diff --git a/src/test/java/org/apache/hadoop/hbase/HBaseClusterTestCase.java b/src/test/java/org/apache/hadoop/hbase/HBaseClusterTestCase.java index 51e6cd920c1..b7abb512f0b 100644 --- a/src/test/java/org/apache/hadoop/hbase/HBaseClusterTestCase.java +++ b/src/test/java/org/apache/hadoop/hbase/HBaseClusterTestCase.java @@ -172,7 +172,7 @@ public abstract class HBaseClusterTestCase extends HBaseTestCase { } super.tearDown(); try { - HConnectionManager.deleteConnectionInfo(conf, true); + HConnectionManager.deleteConnection(conf, true); if (this.cluster != null) { try { this.cluster.shutdown(); diff --git a/src/test/java/org/apache/hadoop/hbase/client/TestHCM.java b/src/test/java/org/apache/hadoop/hbase/client/TestHCM.java index 590ea385db6..1fe4b57f79b 100644 --- a/src/test/java/org/apache/hadoop/hbase/client/TestHCM.java +++ b/src/test/java/org/apache/hadoop/hbase/client/TestHCM.java @@ -19,11 +19,25 @@ */ package org.apache.hadoop.hbase.client; +import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Random; +import java.util.Set; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseConfiguration; import org.apache.hadoop.hbase.HBaseTestingUtility; import org.apache.hadoop.hbase.HRegionLocation; +import org.apache.hadoop.hbase.ZooKeeperConnectionException; import org.apache.hadoop.hbase.util.Bytes; +import org.junit.AfterClass; +import org.junit.Assert; import org.junit.BeforeClass; import org.junit.Test; +import org.mortbay.log.Log; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertNotNull; @@ -32,7 +46,6 @@ import static org.junit.Assert.assertNotNull; * This class is for testing HCM features */ public class TestHCM { - private final static HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility(); private static final byte[] TABLE_NAME = Bytes.toBytes("test"); private static final byte[] FAM_NAM = Bytes.toBytes("f"); @@ -43,6 +56,85 @@ public class TestHCM { TEST_UTIL.startMiniCluster(1); } + @AfterClass public static void tearDownAfterClass() throws Exception { + TEST_UTIL.shutdownMiniCluster(); + } + + /** + * @throws InterruptedException + * @throws IllegalAccessException + * @throws NoSuchFieldException + * @throws ZooKeeperConnectionException + * @throws IllegalArgumentException + * @throws SecurityException + * @see https://issues.apache.org/jira/browse/HBASE-2925 + */ + @Test public void testManyNewConnectionsDoesnotOOME() + throws SecurityException, IllegalArgumentException, + ZooKeeperConnectionException, NoSuchFieldException, IllegalAccessException, + InterruptedException { + createNewConfigurations(); + } + + private static Random _randy = new Random(); + + public static void createNewConfigurations() throws SecurityException, + IllegalArgumentException, NoSuchFieldException, + IllegalAccessException, InterruptedException, ZooKeeperConnectionException { + HConnection last = null; + for (int i = 0; i <= (HConnectionManager.MAX_CACHED_HBASE_INSTANCES * 2); i++) { + // set random key to differentiate the connection from previous ones + Configuration configuration = HBaseConfiguration.create(); + configuration.set("somekey", String.valueOf(_randy.nextInt())); + System.out.println("Hash Code: " + configuration.hashCode()); + HConnection connection = + HConnectionManager.getConnection(configuration); + if (last != null) { + if (last == connection) { + System.out.println("!! Got same connection for once !!"); + } + } + // change the configuration once, and the cached connection is lost forever: + // the hashtable holding the cache won't be able to find its own keys + // to remove them, so the LRU strategy does not work. + configuration.set("someotherkey", String.valueOf(_randy.nextInt())); + last = connection; + Log.info("Cache Size: " + + getHConnectionManagerCacheSize() + ", Valid Keys: " + + getValidKeyCount()); + Thread.sleep(100); + } + Assert.assertEquals(HConnectionManager.MAX_CACHED_HBASE_INSTANCES, + getHConnectionManagerCacheSize()); + Assert.assertEquals(HConnectionManager.MAX_CACHED_HBASE_INSTANCES, + getValidKeyCount()); + } + + private static int getHConnectionManagerCacheSize() + throws SecurityException, NoSuchFieldException, + IllegalArgumentException, IllegalAccessException { + Field cacheField = + HConnectionManager.class.getDeclaredField("HBASE_INSTANCES"); + cacheField.setAccessible(true); + Map cache = (Map) cacheField.get(null); + return cache.size(); + } + + private static int getValidKeyCount() throws SecurityException, + NoSuchFieldException, IllegalArgumentException, + IllegalAccessException { + Field cacheField = + HConnectionManager.class.getDeclaredField("HBASE_INSTANCES"); + cacheField.setAccessible(true); + Map cache = (Map) cacheField.get(null); + List keys = new ArrayList(cache.keySet()); + Set values = new HashSet(); + for (Object key : keys) { + values.add(cache.get(key)); + } + return values.size(); + } + /** * Test that when we delete a location using the first row of a region * that we really delete it. @@ -62,5 +154,4 @@ public class TestHCM { HRegionLocation rl = conn.getCachedLocation(TABLE_NAME, ROW); assertNull("What is this location?? " + rl, rl); } -} - +} \ No newline at end of file diff --git a/src/test/java/org/apache/hadoop/hbase/util/DisabledTestMetaUtils.java b/src/test/java/org/apache/hadoop/hbase/util/DisabledTestMetaUtils.java index 7bf8efbe5f7..3257e952ff3 100644 --- a/src/test/java/org/apache/hadoop/hbase/util/DisabledTestMetaUtils.java +++ b/src/test/java/org/apache/hadoop/hbase/util/DisabledTestMetaUtils.java @@ -50,7 +50,7 @@ public class DisabledTestMetaUtils extends HBaseClusterTestCase { utils.deleteColumn(editTable, Bytes.toBytes(oldColumn)); utils.shutdown(); // Delete again so we go get it all fresh. - HConnectionManager.deleteConnectionInfo(conf, false); + HConnectionManager.deleteConnection(conf, false); // Now assert columns were added and deleted. this.cluster = new MiniHBaseCluster(this.conf, 1); // Now assert columns were added and deleted.