[SPARK-21428] Turn IsolatedClientLoader off while using builtin Hive jars for reusing CliSessionState #18648

yaooqinn · 2017-07-16T16:03:07Z

What changes were proposed in this pull request?

Set isolated to false while using builtin hive jars and SessionState.get returns a CliSessionState instance.

How was this patch tested?

1 Unit Tests
2 Manually verified: hive.exec.strachdir was only created once because of reusing cliSessionState

➜  spark git:(SPARK-21428) ✗ bin/spark-sql --conf spark.sql.hive.metastore.jars=builtin

log4j:WARN No appenders could be found for logger (org.apache.hadoop.util.Shell).
log4j:WARN Please initialize the log4j system properly.
log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info.
Using Spark's default log4j profile: org/apache/spark/log4j-defaults.properties
17/07/16 23:59:27 WARN NativeCodeLoader: Unable to load native-hadoop library for your platform... using builtin-java classes where applicable
17/07/16 23:59:27 INFO HiveMetaStore: 0: Opening raw store with implemenation class:org.apache.hadoop.hive.metastore.ObjectStore
17/07/16 23:59:27 INFO ObjectStore: ObjectStore, initialize called
17/07/16 23:59:28 INFO Persistence: Property hive.metastore.integral.jdo.pushdown unknown - will be ignored
17/07/16 23:59:28 INFO Persistence: Property datanucleus.cache.level2 unknown - will be ignored
17/07/16 23:59:29 INFO ObjectStore: Setting MetaStore object pin classes with hive.metastore.cache.pinobjtypes="Table,StorageDescriptor,SerDeInfo,Partition,Database,Type,FieldSchema,Order"
17/07/16 23:59:30 INFO Datastore: The class "org.apache.hadoop.hive.metastore.model.MFieldSchema" is tagged as "embedded-only" so does not have its own datastore table.
17/07/16 23:59:30 INFO Datastore: The class "org.apache.hadoop.hive.metastore.model.MOrder" is tagged as "embedded-only" so does not have its own datastore table.
17/07/16 23:59:31 INFO Datastore: The class "org.apache.hadoop.hive.metastore.model.MFieldSchema" is tagged as "embedded-only" so does not have its own datastore table.
17/07/16 23:59:31 INFO Datastore: The class "org.apache.hadoop.hive.metastore.model.MOrder" is tagged as "embedded-only" so does not have its own datastore table.
17/07/16 23:59:31 INFO MetaStoreDirectSql: Using direct SQL, underlying DB is DERBY
17/07/16 23:59:31 INFO ObjectStore: Initialized ObjectStore
17/07/16 23:59:31 WARN ObjectStore: Version information not found in metastore. hive.metastore.schema.verification is not enabled so recording the schema version 1.2.0
17/07/16 23:59:31 WARN ObjectStore: Failed to get database default, returning NoSuchObjectException
17/07/16 23:59:32 INFO HiveMetaStore: Added admin role in metastore
17/07/16 23:59:32 INFO HiveMetaStore: Added public role in metastore
17/07/16 23:59:32 INFO HiveMetaStore: No user is added in admin role, since config is empty
17/07/16 23:59:32 INFO HiveMetaStore: 0: get_all_databases
17/07/16 23:59:32 INFO audit: ugi=Kent	ip=unknown-ip-addr	cmd=get_all_databases
17/07/16 23:59:32 INFO HiveMetaStore: 0: get_functions: db=default pat=*
17/07/16 23:59:32 INFO audit: ugi=Kent	ip=unknown-ip-addr	cmd=get_functions: db=default pat=*
17/07/16 23:59:32 INFO Datastore: The class "org.apache.hadoop.hive.metastore.model.MResourceUri" is tagged as "embedded-only" so does not have its own datastore table.
17/07/16 23:59:32 INFO SessionState: Created local directory: /var/folders/k2/04p4k4ws73l6711h_mz2_tq00000gn/T/beea7261-221a-4711-89e8-8b12a9d37370_resources
17/07/16 23:59:32 INFO SessionState: Created HDFS directory: /tmp/hive/Kent/beea7261-221a-4711-89e8-8b12a9d37370
17/07/16 23:59:32 INFO SessionState: Created local directory: /var/folders/k2/04p4k4ws73l6711h_mz2_tq00000gn/T/Kent/beea7261-221a-4711-89e8-8b12a9d37370
17/07/16 23:59:32 INFO SessionState: Created HDFS directory: /tmp/hive/Kent/beea7261-221a-4711-89e8-8b12a9d37370/_tmp_space.db
17/07/16 23:59:32 INFO SparkContext: Running Spark version 2.3.0-SNAPSHOT
17/07/16 23:59:32 INFO SparkContext: Submitted application: SparkSQL::10.0.0.8
17/07/16 23:59:32 INFO SecurityManager: Changing view acls to: Kent
17/07/16 23:59:32 INFO SecurityManager: Changing modify acls to: Kent
17/07/16 23:59:32 INFO SecurityManager: Changing view acls groups to:
17/07/16 23:59:32 INFO SecurityManager: Changing modify acls groups to:
17/07/16 23:59:32 INFO SecurityManager: SecurityManager: authentication disabled; ui acls disabled; users  with view permissions: Set(Kent); groups with view permissions: Set(); users  with modify permissions: Set(Kent); groups with modify permissions: Set()
17/07/16 23:59:33 INFO Utils: Successfully started service 'sparkDriver' on port 51889.
17/07/16 23:59:33 INFO SparkEnv: Registering MapOutputTracker
17/07/16 23:59:33 INFO SparkEnv: Registering BlockManagerMaster
17/07/16 23:59:33 INFO BlockManagerMasterEndpoint: Using org.apache.spark.storage.DefaultTopologyMapper for getting topology information
17/07/16 23:59:33 INFO BlockManagerMasterEndpoint: BlockManagerMasterEndpoint up
17/07/16 23:59:33 INFO DiskBlockManager: Created local directory at /private/var/folders/k2/04p4k4ws73l6711h_mz2_tq00000gn/T/blockmgr-9cfae28a-01e9-4c73-a1f1-f76fa52fc7a5
17/07/16 23:59:33 INFO MemoryStore: MemoryStore started with capacity 366.3 MB
17/07/16 23:59:33 INFO SparkEnv: Registering OutputCommitCoordinator
17/07/16 23:59:33 INFO Utils: Successfully started service 'SparkUI' on port 4040.
17/07/16 23:59:33 INFO SparkUI: Bound SparkUI to 0.0.0.0, and started at http://10.0.0.8:4040
17/07/16 23:59:33 INFO Executor: Starting executor ID driver on host localhost
17/07/16 23:59:33 INFO Utils: Successfully started service 'org.apache.spark.network.netty.NettyBlockTransferService' on port 51890.
17/07/16 23:59:33 INFO NettyBlockTransferService: Server created on 10.0.0.8:51890
17/07/16 23:59:33 INFO BlockManager: Using org.apache.spark.storage.RandomBlockReplicationPolicy for block replication policy
17/07/16 23:59:33 INFO BlockManagerMaster: Registering BlockManager BlockManagerId(driver, 10.0.0.8, 51890, None)
17/07/16 23:59:33 INFO BlockManagerMasterEndpoint: Registering block manager 10.0.0.8:51890 with 366.3 MB RAM, BlockManagerId(driver, 10.0.0.8, 51890, None)
17/07/16 23:59:33 INFO BlockManagerMaster: Registered BlockManager BlockManagerId(driver, 10.0.0.8, 51890, None)
17/07/16 23:59:33 INFO BlockManager: Initialized BlockManager: BlockManagerId(driver, 10.0.0.8, 51890, None)
17/07/16 23:59:34 INFO SharedState: Setting hive.metastore.warehouse.dir ('null') to the value of spark.sql.warehouse.dir ('file:/Users/Kent/Documents/spark/spark-warehouse').
17/07/16 23:59:34 INFO SharedState: Warehouse path is 'file:/Users/Kent/Documents/spark/spark-warehouse'.
17/07/16 23:59:34 INFO HiveUtils: Initializing HiveMetastoreConnection version 1.2.1 using Spark classes.
17/07/16 23:59:34 INFO HiveClientImpl: Warehouse location for Hive client (version 1.2.2) is /user/hive/warehouse
17/07/16 23:59:34 INFO HiveMetaStore: 0: get_database: default
17/07/16 23:59:34 INFO audit: ugi=Kent	ip=unknown-ip-addr	cmd=get_database: default
17/07/16 23:59:34 INFO HiveClientImpl: Warehouse location for Hive client (version 1.2.2) is /user/hive/warehouse
17/07/16 23:59:34 INFO HiveMetaStore: 0: get_database: global_temp
17/07/16 23:59:34 INFO audit: ugi=Kent	ip=unknown-ip-addr	cmd=get_database: global_temp
17/07/16 23:59:34 WARN ObjectStore: Failed to get database global_temp, returning NoSuchObjectException
17/07/16 23:59:34 INFO HiveClientImpl: Warehouse location for Hive client (version 1.2.2) is /user/hive/warehouse
17/07/16 23:59:34 INFO StateStoreCoordinatorRef: Registered StateStoreCoordinator endpoint
spark-sql>

cc @cloud-fan @gatorsmile

yaooqinn · 2017-08-04T08:30:12Z

ping @gatorsmile could you help to review this？

cloud-fan · 2017-08-07T16:38:25Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala

@@ -312,7 +323,7 @@ private[spark] object HiveUtils extends Logging {
        hadoopConf = hadoopConf,
        execJars = jars.toSeq,
        config = configurations,
-        isolationOn = true,
+        isolationOn = isCliSessionState(),


can you explain more about this? Why do we need to do this?

@cloud-fan According to HiveClientImpl.scala#L140, the cliSessionState shall be reused. But because of IsolateClientClassloader, originalState will be null. Then it always goes to the else branch to create and start a new session.SessionState

cloud-fan · 2017-08-08T03:37:00Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala

+            // properties and then loaded by SparkConf
+            sysProps.put("spark.yarn.keytab", args.keytab)
+            sysProps.put("spark.yarn.principal", args.principal)
+          case Failure(exception) => throw exception


we can just write

SparkHadoopUtil.get.loginUserFromKeytab(args.principal, args.keytab) // the comments ... sysProps.put("spark.yarn.keytab", args.keytab) sysProps.put("spark.yarn.principal", args.principal)

cloud-fan · 2017-08-08T03:38:56Z

...tserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveCliSessionStateSuite.scala

+    val sparkConf = new SparkConf()
+    val hadoopConf = SparkHadoopUtil.get.newConfiguration(sparkConf)
+    val hiveClient = HiveUtils.newClientForMetadata(sparkConf, hadoopConf)
+    assert((hiveClient.toString == s1.toString) === expected)


is it safe to just compare toString result?

cloud-fan · 2017-08-08T03:40:39Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

@@ -269,6 +232,8 @@ private[hive] class HiveClientImpl(
    }
  }

+  override def toString: String = state.toString


This is not a reasonable toString implementation for HiveClientImpl

May i add def getState(): SessionState to HiveClientImpl?

cloud-fan · 2017-08-08T11:24:37Z

OK to test

cloud-fan · 2017-08-08T11:26:25Z

...tserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveCliSessionStateSuite.scala

+    val s1 = SessionState.get
+    val sparkConf = new SparkConf()
+    val hadoopConf = SparkHadoopUtil.get.newConfiguration(sparkConf)
+    val s2 = HiveUtils.newClientForMetadata(sparkConf, hadoopConf).getState


how about HiveUtils.newClientForMetadata(sparkConf, hadoopConf).asInstanceOf[HiveClientImpl].state? then we don't need to add getState

with IsolateClientClassload, this seems to cause ClassCastException

weird, HiveClientImpl is the only implementation of the HiveClient interface.

cloud-fan · 2017-08-09T05:29:05Z

ok to test

SparkQA · 2017-08-09T07:04:50Z

Test build #80442 has finished for PR 18648 at commit 6c0bf70.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2017-08-09T08:13:46Z

retest this please

jiangxb1987 · 2017-08-09T10:49:45Z

core/src/main/scala/org/apache/spark/deploy/SparkHadoopUtil.scala

+  def loginUserFromKeytab(principalName: String, keytabFilename: String): Unit = {
+    if (!new File(keytabFilename).exists()) {
+      throw new SparkException(s"Keytab file: ${keytabFilename}" +
+        " specified in spark.yarn.keytab does not exist")


nit: To be general, let's not mention the config name spark.yarn.keytab here.

ok,notice that

jiangxb1987 · 2017-08-09T11:14:33Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

+    (hadoopConf.iterator().asScala.map(kv => kv.getKey -> kv.getValue)
+      ++ sparkConf.getAll.toMap ++ extraConfig).foreach { case (k, v) =>
+      if (k.toLowerCase(Locale.ROOT).contains("password")) {
+        logDebug(s"Applying Spark config to Hive Conf: $k=xxx")


This may also be Hadoop/Hive or extra config.

ok, thanks.

jiangxb1987 · 2017-08-09T11:22:01Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala

@@ -229,6 +230,17 @@ private[spark] object HiveUtils extends Logging {
    }.toMap
  }

+  def isCliSessionState(): Boolean = {


nit: Should add comment for this method.

jiangxb1987 · 2017-08-09T11:26:14Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

-        temp = temp.getSuperclass
+    if (clientLoader.isolationOn) {
+      // Switch to the initClassLoader.
+      Thread.currentThread().setContextClassLoader(initClassLoader)


Is the behavior change safe here? Previously, we switch the context ClassLoader for both conditions, while in this PR we only do that if isolationOn is true.

when isolation Off, we just switch a classloader to itself

If SessionState.get() is None, we should still call newState() and init from initClassLoader, should we also switch in that case?

If SessionState.get be null, then the IsolateOn will be turned on always. Only if we call SessionState.detachSession, will this happens?

A user app new an CliSessionState instance with built in hive jars to trigger isolate off, then it detach this state, and then new a hive client again, this time isolate off and SessionState.get() will be None, newState() will be called without changing the classloader, I think this is OK, because we never create a isolate class loader from beginning to end.

SparkQA · 2017-08-09T11:33:56Z

Test build #80448 has finished for PR 18648 at commit 51fac11.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jiangxb1987 · 2017-08-09T14:39:05Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

-    } finally {
-      Thread.currentThread().setContextClassLoader(original)
+    } else {
+      Option(SessionState.get()).getOrElse(newState())


Since SessionState.get() won't be None here, we can simplify the code to

SessionState.get()

, and add comment above to issue the reason of doing this.

In the condition I mentioned above, I think this should be kept

SparkQA · 2017-08-09T19:04:50Z

Test build #80458 has finished for PR 18648 at commit c50a32f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-08-09T19:06:43Z

Test build #80460 has finished for PR 18648 at commit c6ed2d7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2017-08-10T01:57:19Z

ping @jiangxb1987 @cloud-fan anymore suggestions？

yaooqinn · 2017-08-13T14:10:43Z

ping @jiangxb1987 @cloud-fan again

jiangxb1987 · 2017-08-13T15:11:05Z

LGTM

yaooqinn · 2017-08-14T02:45:36Z

ping @cloud-fan would you take another look？

yaooqinn · 2017-08-16T08:50:31Z

@jiangxb1987 Could this pr be merged？

yaooqinn · 2017-08-17T06:41:44Z

@cloud-fan

cloud-fan · 2017-08-17T16:25:11Z

LGTM, merging to master!

viirya · 2017-09-20T02:48:37Z

The description is not clear, at least I get understood after diving into the code changes.

set isolateOn to false while using builtin hive jars

394b471

yaooqinn changed the title ~~[SPARK-21428] Set IsolatedClientLoader off while using builtin Hive jars for reusing CliSessionState~~ [SPARK-21428] Turn IsolatedClientLoader off while using builtin Hive jars for reusing CliSessionState Jul 17, 2017

add unit tests

62049f5

yaooqinn mentioned this pull request Jul 31, 2017

[SPARK-21637][SPARK-21451][SQL]get spark.hadoop.* properties from sysProps to hiveconf #18668

Closed

refacting code

d127f96

cloud-fan reviewed Aug 7, 2017

View reviewed changes

yaooqinn added 2 commits August 8, 2017 10:16

move suites to thriftserver module for bannedDependencies

d18bcc0

fix ut reduplicated derby metastore_db

341964e

cloud-fan reviewed Aug 8, 2017

View reviewed changes

review change

6c0bf70

cloud-fan reviewed Aug 8, 2017

View reviewed changes

ut affect each other

51fac11

jiangxb1987 reviewed Aug 9, 2017

View reviewed changes

yaooqinn added 2 commits August 9, 2017 23:45

thanks for reviewing

c50a32f

typo

c6ed2d7

asfgit closed this in b83b502 Aug 17, 2017

yaooqinn mentioned this pull request Sep 19, 2017

Revert "[SPARK-21428] Turn IsolatedClientLoader off while using builtin Hive jars for reusing CliSessionState #19273

Closed

[SPARK-21428] Turn IsolatedClientLoader off while using builtin Hive jars for reusing CliSessionState #18648

[SPARK-21428] Turn IsolatedClientLoader off while using builtin Hive jars for reusing CliSessionState #18648

Conversation

yaooqinn commented Jul 16, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

yaooqinn commented Aug 4, 2017

Choose a reason for hiding this comment

yaooqinn Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Aug 9, 2017

SparkQA commented Aug 9, 2017

yaooqinn commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaooqinn Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Aug 9, 2017

jiangxb1987 Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 9, 2017

SparkQA commented Aug 9, 2017

yaooqinn commented Aug 10, 2017

yaooqinn commented Aug 13, 2017

jiangxb1987 commented Aug 13, 2017

yaooqinn commented Aug 14, 2017

yaooqinn commented Aug 16, 2017

yaooqinn commented Aug 17, 2017

cloud-fan commented Aug 17, 2017

viirya commented Sep 20, 2017

yaooqinn commented Jul 16, 2017 •

edited

Loading

yaooqinn Aug 8, 2017 •

edited

Loading

yaooqinn Aug 9, 2017 •

edited

Loading

jiangxb1987 Aug 9, 2017 •

edited

Loading