HIVE-14187: JDOPersistenceManager objects remain cached if MetaStoreClient#close is not called (Mohit Sabharwal, reviewed by Vaibhav Gumasha, via Sergio Pena)
Project: http://git-wip-us.apache.org/repos/asf/hive/repo Commit: http://git-wip-us.apache.org/repos/asf/hive/commit/e045c5a5 Tree: http://git-wip-us.apache.org/repos/asf/hive/tree/e045c5a5 Diff: http://git-wip-us.apache.org/repos/asf/hive/diff/e045c5a5 Branch: refs/heads/branch-2.1 Commit: e045c5a572498f0d84f363ebc6075f0f0fba2e51 Parents: fb99e3e Author: Mohit Sabharwal <[email protected]> Authored: Fri Jul 15 16:46:04 2016 -0500 Committer: Sergio Pena <[email protected]> Committed: Wed Sep 28 15:54:42 2016 -0500 ---------------------------------------------------------------------- .../hive/metastore/TestHiveMetaStore.java | 51 ++++++++++++++++++++ .../hive/metastore/TestRemoteHiveMetaStore.java | 10 ++-- .../hive/metastore/TestSetUGIOnOnlyClient.java | 4 +- .../hive/metastore/TestSetUGIOnOnlyServer.java | 4 +- .../hadoop/hive/metastore/HiveMetaStore.java | 35 ++++++++------ .../hive/metastore/HiveMetaStoreClient.java | 6 +++ 6 files changed, 88 insertions(+), 22 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hive/blob/e045c5a5/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java ---------------------------------------------------------------------- diff --git a/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java b/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java index 9a86fdd..1697780 100644 --- a/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java +++ b/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java @@ -18,6 +18,7 @@ package org.apache.hadoop.hive.metastore; +import java.lang.reflect.Field; import java.sql.Connection; import java.sql.PreparedStatement; import java.sql.SQLException; @@ -33,6 +34,8 @@ import java.util.Set; import junit.framework.TestCase; +import org.datanucleus.api.jdo.JDOPersistenceManager; +import org.datanucleus.api.jdo.JDOPersistenceManagerFactory; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.apache.hadoop.fs.FileSystem; @@ -3166,6 +3169,54 @@ public abstract class TestHiveMetaStore extends TestCase { client.close(); } + public void testJDOPersistanceManagerCleanup() throws Exception { + if (isThriftClient == false) { + return; + } + + int numObjectsBeforeClose = getJDOPersistanceManagerCacheSize(); + HiveMetaStoreClient closingClient = new HiveMetaStoreClient(hiveConf); + closingClient.getAllDatabases(); + closingClient.close(); + Thread.sleep(5 * 1000); // give HMS time to handle close request + int numObjectsAfterClose = getJDOPersistanceManagerCacheSize(); + Assert.assertTrue(numObjectsBeforeClose == numObjectsAfterClose); + + HiveMetaStoreClient nonClosingClient = new HiveMetaStoreClient(hiveConf); + nonClosingClient.getAllDatabases(); + // Drop connection without calling close. HMS thread deleteContext + // will trigger cleanup + nonClosingClient.getTTransport().close(); + Thread.sleep(5 * 1000); + int numObjectsAfterDroppedConnection = getJDOPersistanceManagerCacheSize(); + Assert.assertTrue(numObjectsAfterClose == numObjectsAfterDroppedConnection); + } + + private static int getJDOPersistanceManagerCacheSize() { + JDOPersistenceManagerFactory jdoPmf; + Set<JDOPersistenceManager> pmCacheObj; + Field pmCache; + Field pmf; + try { + pmf = ObjectStore.class.getDeclaredField("pmf"); + if (pmf != null) { + pmf.setAccessible(true); + jdoPmf = (JDOPersistenceManagerFactory) pmf.get(null); + pmCache = JDOPersistenceManagerFactory.class.getDeclaredField("pmCache"); + if (pmCache != null) { + pmCache.setAccessible(true); + pmCacheObj = (Set<JDOPersistenceManager>) pmCache.get(jdoPmf); + if (pmCacheObj != null) { + return pmCacheObj.size(); + } + } + } + } catch (Exception ex) { + System.out.println(ex); + } + return -1; + } + private HiveMetaHookLoader getHookLoader() { HiveMetaHookLoader hookLoader = new HiveMetaHookLoader() { @Override http://git-wip-us.apache.org/repos/asf/hive/blob/e045c5a5/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestRemoteHiveMetaStore.java ---------------------------------------------------------------------- diff --git a/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestRemoteHiveMetaStore.java b/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestRemoteHiveMetaStore.java index 491d093..faea0e4 100644 --- a/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestRemoteHiveMetaStore.java +++ b/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestRemoteHiveMetaStore.java @@ -25,6 +25,7 @@ import org.apache.hadoop.hive.shims.ShimLoader; public class TestRemoteHiveMetaStore extends TestHiveMetaStore { private static boolean isServerStarted = false; + private static int port; public TestRemoteHiveMetaStore() { super(); @@ -37,21 +38,22 @@ public class TestRemoteHiveMetaStore extends TestHiveMetaStore { if (isServerStarted) { assertNotNull("Unable to connect to the MetaStore server", client); + hiveConf.setVar(HiveConf.ConfVars.METASTOREURIS, "thrift://localhost:" + port); return; } - int port = MetaStoreUtils.findFreePort(); + port = MetaStoreUtils.findFreePort(); System.out.println("Starting MetaStore Server on port " + port); MetaStoreUtils.startMetaStore(port, ShimLoader.getHadoopThriftAuthBridge()); isServerStarted = true; // This is default case with setugi off for both client and server - createClient(false, port); + createClient(false); } - protected void createClient(boolean setugi, int port) throws Exception { + protected void createClient(boolean setugi) throws Exception { hiveConf.setVar(HiveConf.ConfVars.METASTOREURIS, "thrift://localhost:" + port); hiveConf.setBoolVar(ConfVars.METASTORE_EXECUTE_SET_UGI,setugi); client = new HiveMetaStoreClient(hiveConf); } -} +} \ No newline at end of file http://git-wip-us.apache.org/repos/asf/hive/blob/e045c5a5/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestSetUGIOnOnlyClient.java ---------------------------------------------------------------------- diff --git a/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestSetUGIOnOnlyClient.java b/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestSetUGIOnOnlyClient.java index 2c6d567..29768c1 100644 --- a/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestSetUGIOnOnlyClient.java +++ b/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestSetUGIOnOnlyClient.java @@ -21,8 +21,8 @@ package org.apache.hadoop.hive.metastore; public class TestSetUGIOnOnlyClient extends TestRemoteHiveMetaStore{ @Override - protected void createClient(boolean setugi, int port) throws Exception { + protected void createClient(boolean setugi) throws Exception { // turn it on for client. - super.createClient(true, port); + super.createClient(true); } } http://git-wip-us.apache.org/repos/asf/hive/blob/e045c5a5/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestSetUGIOnOnlyServer.java ---------------------------------------------------------------------- diff --git a/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestSetUGIOnOnlyServer.java b/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestSetUGIOnOnlyServer.java index 6c3fbf6..4a46f75 100644 --- a/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestSetUGIOnOnlyServer.java +++ b/itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestSetUGIOnOnlyServer.java @@ -21,8 +21,8 @@ package org.apache.hadoop.hive.metastore; public class TestSetUGIOnOnlyServer extends TestSetUGIOnBothClientServer { @Override - protected void createClient(boolean setugi, int port) throws Exception { + protected void createClient(boolean setugi) throws Exception { // It is turned on for both client and server because of super class. Turn it off for client. - super.createClient(false, port); + super.createClient(false); } } http://git-wip-us.apache.org/repos/asf/hive/blob/e045c5a5/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java ---------------------------------------------------------------------- diff --git a/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java b/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java index 66bf23c..e46e4d1a 100644 --- a/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java +++ b/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java @@ -281,7 +281,7 @@ public class HiveMetaStore extends ThriftHiveMetastore { } }; - private final void logAuditEvent(String cmd) { + private static final void logAuditEvent(String cmd) { if (cmd == null) { return; } @@ -304,7 +304,7 @@ public class HiveMetaStore extends ThriftHiveMetastore { address, cmd).toString()); } - String getIPAddress() { + private static String getIPAddress() { if (useSasl) { if (saslServer != null && saslServer.getRemoteAddress() != null) { return saslServer.getRemoteAddress().getHostAddress(); @@ -747,7 +747,7 @@ public class HiveMetaStore extends ThriftHiveMetastore { } } - private void logInfo(String m) { + private static void logInfo(String m) { LOG.info(threadLocalId.get().toString() + ": " + m); logAuditEvent(m); } @@ -820,17 +820,7 @@ public class HiveMetaStore extends ThriftHiveMetastore { @Override public void shutdown() { - logInfo("Metastore shutdown started..."); - RawStore ms = threadLocalMS.get(); - if (ms != null) { - try { - ms.shutdown(); - } finally { - threadLocalConf.remove(); - threadLocalMS.remove(); - } - } - logInfo("Metastore shutdown complete."); + cleanupRawStore(); } @Override @@ -6786,6 +6776,9 @@ public class HiveMetaStore extends ThriftHiveMetastore { } catch (Exception e) { LOG.warn("Error Reporting Metastore close connection to Metrics system", e); } + // If the IMetaStoreClient#close was called, HMSHandler#shutdown would have already + // cleaned up thread local RawStore. Otherwise, do it now. + cleanupRawStore(); } @Override @@ -6813,6 +6806,20 @@ public class HiveMetaStore extends ThriftHiveMetastore { } } + private static void cleanupRawStore() { + RawStore rs = HMSHandler.getRawStore(); + if (rs != null) { + HMSHandler.logInfo("Cleaning up thread local RawStore..."); + try { + rs.shutdown(); + } finally { + HMSHandler.threadLocalConf.remove(); + HMSHandler.removeRawStore(); + } + HMSHandler.logInfo("Done cleaning up thread local RawStore"); + } + } + private static void signalOtherThreadsToStart(final TServer server, final Lock startLock, final Condition startCondition, final AtomicBoolean startedServing) { http://git-wip-us.apache.org/repos/asf/hive/blob/e045c5a5/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java ---------------------------------------------------------------------- diff --git a/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java b/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java index 0b2f77b..129512d 100644 --- a/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java +++ b/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java @@ -18,6 +18,7 @@ package org.apache.hadoop.hive.metastore; +import com.google.common.annotations.VisibleForTesting; import org.apache.hadoop.hive.common.ObjectPair; import org.apache.hadoop.hive.common.ValidTxnList; import org.apache.hadoop.hive.common.classification.InterfaceAudience; @@ -324,6 +325,11 @@ public class HiveMetaStoreClient implements IMetaStoreClient { metastoreUris[index] = tmp; } + @VisibleForTesting + public TTransport getTTransport() { + return transport; + } + @Override public boolean isLocalMetaStore() { return localMetaStore;
