You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kyuubi.apache.org by bo...@apache.org on 2023/05/29 08:56:07 UTC
[kyuubi] branch master updated: [KYUUBI #4887] Refactor and add ut for ClassUtils
This is an automated email from the ASF dual-hosted git repository.
bowenliang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kyuubi.git
The following commit(s) were added to refs/heads/master by this push:
new e15585106 [KYUUBI #4887] Refactor and add ut for ClassUtils
e15585106 is described below
commit e155851069deeb8bc55943b72ca9cef66e1012d8
Author: liangbowen <li...@gf.com.cn>
AuthorDate: Mon May 29 16:55:56 2023 +0800
[KYUUBI #4887] Refactor and add ut for ClassUtils
### _Why are the changes needed?_
- add unit tests for `ClassUtils`
- refactor `ClassUtils.createInstance` method with DynConstructors
- move `classIsLoadable` method to `ReflectUtils.isClassLoadable`, refeactor to use DynClasses
### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request
Closes #4887 from bowenliang123/classutil.
Closes #4887
c39f9a0a4 [liangbowen] simplify ut
633e21ddc [liangbowen] Refactor and add ut for ClassUtils
Authored-by: liangbowen <li...@gf.com.cn>
Signed-off-by: liangbowen <li...@gf.com.cn>
---
.../main/scala/org/apache/spark/ui/EngineTab.scala | 23 +++----
.../src/main/scala/org/apache/kyuubi/Logging.scala | 4 +-
.../scala/org/apache/kyuubi/util/ClassUtils.scala | 33 +++-------
.../org/apache/kyuubi/util/ClassUtilsSuite.scala | 71 ++++++++++++++++++++++
.../apache/kyuubi/util/reflect/ReflectUtils.scala | 13 ++++
.../kyuubi/util/reflect/ReflectUtilsSuite.scala | 29 +++++++++
6 files changed, 133 insertions(+), 40 deletions(-)
diff --git a/externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/ui/EngineTab.scala b/externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/ui/EngineTab.scala
index b7cebbd97..14ef3d3fd 100644
--- a/externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/ui/EngineTab.scala
+++ b/externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/ui/EngineTab.scala
@@ -26,7 +26,7 @@ import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.engine.spark.SparkSQLEngine
import org.apache.kyuubi.engine.spark.events.EngineEventsStore
import org.apache.kyuubi.service.ServiceState
-import org.apache.kyuubi.util.ClassUtils
+import org.apache.kyuubi.util.reflect.DynClasses
/**
* Note that [[SparkUITab]] is private for Spark
@@ -62,8 +62,13 @@ case class EngineTab(
sparkUI.foreach { ui =>
try {
- // Spark shade the jetty package so here we use reflection
- val sparkServletContextHandlerClz = loadSparkServletContextHandler
+ // [KYUUBI #3627]: the official spark release uses the shaded and relocated jetty classes,
+ // but if we use sbt to build for testing, e.g. docker image, it still uses the vanilla
+ // jetty classes.
+ val sparkServletContextHandlerClz = DynClasses.builder()
+ .impl("org.sparkproject.jetty.servlet.ServletContextHandler")
+ .impl("org.eclipse.jetty.servlet.ServletContextHandler")
+ .build()
val attachHandlerMethod = Class.forName("org.apache.spark.ui.SparkUI")
.getMethod("attachHandler", sparkServletContextHandlerClz)
val createRedirectHandlerMethod = Class.forName("org.apache.spark.ui.JettyUtils")
@@ -105,18 +110,6 @@ case class EngineTab(
cause)
}
- private def loadSparkServletContextHandler: Class[_] = {
- // [KYUUBI #3627]: the official spark release uses the shaded and relocated jetty classes,
- // but if use sbt to build for testing, e.g. docker image, it still uses vanilla jetty classes.
- val shaded = "org.sparkproject.jetty.servlet.ServletContextHandler"
- val vanilla = "org.eclipse.jetty.servlet.ServletContextHandler"
- if (ClassUtils.classIsLoadable(shaded)) {
- Class.forName(shaded)
- } else {
- Class.forName(vanilla)
- }
- }
-
def handleKillRequest(request: HttpServletRequest): Unit = {
if (killEnabled && engine.isDefined && engine.get.getServiceState != ServiceState.STOPPED) {
engine.get.stop()
diff --git a/kyuubi-common/src/main/scala/org/apache/kyuubi/Logging.scala b/kyuubi-common/src/main/scala/org/apache/kyuubi/Logging.scala
index 1df598132..82c1bd45a 100644
--- a/kyuubi-common/src/main/scala/org/apache/kyuubi/Logging.scala
+++ b/kyuubi-common/src/main/scala/org/apache/kyuubi/Logging.scala
@@ -23,7 +23,7 @@ import org.apache.logging.log4j.core.config.DefaultConfiguration
import org.slf4j.{Logger, LoggerFactory}
import org.slf4j.bridge.SLF4JBridgeHandler
-import org.apache.kyuubi.util.ClassUtils
+import org.apache.kyuubi.util.reflect.ReflectUtils
/**
* Simple version of logging adopted from Apache Spark.
@@ -148,7 +148,7 @@ object Logging {
isInterpreter: Boolean,
loggerName: String,
logger: => Logger): Unit = {
- if (ClassUtils.classIsLoadable("org.slf4j.bridge.SLF4JBridgeHandler")) {
+ if (ReflectUtils.isClassLoadable("org.slf4j.bridge.SLF4JBridgeHandler")) {
// Handles configuring the JUL -> SLF4J bridge
SLF4JBridgeHandler.removeHandlersForRootLogger()
SLF4JBridgeHandler.install()
diff --git a/kyuubi-common/src/main/scala/org/apache/kyuubi/util/ClassUtils.scala b/kyuubi-common/src/main/scala/org/apache/kyuubi/util/ClassUtils.scala
index bcbfdabfb..d8eda3426 100644
--- a/kyuubi-common/src/main/scala/org/apache/kyuubi/util/ClassUtils.scala
+++ b/kyuubi-common/src/main/scala/org/apache/kyuubi/util/ClassUtils.scala
@@ -17,10 +17,9 @@
package org.apache.kyuubi.util
-import scala.util.Try
-
import org.apache.kyuubi.KyuubiException
import org.apache.kyuubi.config.KyuubiConf
+import org.apache.kyuubi.util.reflect._
object ClassUtils {
@@ -34,28 +33,16 @@ object ClassUtils {
*/
def createInstance[T](className: String, expected: Class[T], conf: KyuubiConf): T = {
val classLoader = Thread.currentThread.getContextClassLoader
- val cls = Class.forName(className, true, classLoader)
- cls match {
- case clazz if expected.isAssignableFrom(cls) =>
- val confConstructor = clazz.getConstructors.exists(p => {
- val params = p.getParameterTypes
- params.length == 1 && classOf[KyuubiConf].isAssignableFrom(params(0))
- })
- if (confConstructor) {
- clazz.getConstructor(classOf[KyuubiConf]).newInstance(conf)
- .asInstanceOf[T]
- } else {
- clazz.newInstance().asInstanceOf[T]
- }
- case _ => throw new KyuubiException(
- s"$className must extend of ${expected.getName}")
+ try {
+ DynConstructors.builder(expected).loader(classLoader)
+ .impl(className, classOf[KyuubiConf])
+ .impl(className)
+ .buildChecked[T]()
+ .newInstance(conf)
+ } catch {
+ case e: Exception =>
+ throw new KyuubiException(s"$className must extend of ${expected.getName}", e)
}
}
- /** Determines whether the provided class is loadable. */
- def classIsLoadable(
- clazz: String,
- cl: ClassLoader = Thread.currentThread().getContextClassLoader): Boolean = {
- Try { Class.forName(clazz, false, cl) }.isSuccess
- }
}
diff --git a/kyuubi-common/src/test/scala/org/apache/kyuubi/util/ClassUtilsSuite.scala b/kyuubi-common/src/test/scala/org/apache/kyuubi/util/ClassUtilsSuite.scala
new file mode 100644
index 000000000..cda638b0d
--- /dev/null
+++ b/kyuubi-common/src/test/scala/org/apache/kyuubi/util/ClassUtilsSuite.scala
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.util
+
+import org.apache.kyuubi.KyuubiFunSuite
+import org.apache.kyuubi.config.KyuubiConf
+
+class ClassUtilsSuite extends KyuubiFunSuite {
+
+ private val _conf = KyuubiConf()
+
+ test("create instance with zero-arg arg") {
+ val instance = ClassUtils.createInstance[SomeProvider](
+ "org.apache.kyuubi.util.ProviderA",
+ classOf[SomeProvider],
+ _conf)
+
+ assert(instance != null)
+ assert(instance.isInstanceOf[SomeProvider])
+ assert(instance.isInstanceOf[ProviderA])
+ }
+
+ test("create instance with kyuubi conf") {
+ val instance = ClassUtils.createInstance[SomeProvider](
+ "org.apache.kyuubi.util.ProviderB",
+ classOf[SomeProvider],
+ _conf)
+ assert(instance != null)
+ assert(instance.isInstanceOf[SomeProvider])
+ assert(instance.isInstanceOf[ProviderB])
+ assert(instance.asInstanceOf[ProviderB].getConf != null)
+ }
+
+ test("create instance of inherited class with kyuubi conf") {
+ val instance = ClassUtils.createInstance[SomeProvider](
+ "org.apache.kyuubi.util.ProviderC",
+ classOf[SomeProvider],
+ _conf)
+ assert(instance != null)
+ assert(instance.isInstanceOf[SomeProvider])
+ assert(instance.isInstanceOf[ProviderB])
+ assert(instance.isInstanceOf[ProviderC])
+ assert(instance.asInstanceOf[ProviderC].getConf != null)
+ }
+
+}
+
+trait SomeProvider {}
+
+class ProviderA extends SomeProvider {}
+
+class ProviderB(conf: KyuubiConf) extends SomeProvider {
+ def getConf: KyuubiConf = conf
+}
+
+class ProviderC(conf: KyuubiConf) extends ProviderB(conf) {}
diff --git a/kyuubi-util-scala/src/main/scala/org/apache/kyuubi/util/reflect/ReflectUtils.scala b/kyuubi-util-scala/src/main/scala/org/apache/kyuubi/util/reflect/ReflectUtils.scala
index 843b8489a..2c46c775b 100644
--- a/kyuubi-util-scala/src/main/scala/org/apache/kyuubi/util/reflect/ReflectUtils.scala
+++ b/kyuubi-util-scala/src/main/scala/org/apache/kyuubi/util/reflect/ReflectUtils.scala
@@ -21,6 +21,19 @@ import scala.util.{Failure, Success, Try}
object ReflectUtils {
+ /**
+ * Determines whether the provided class is loadable
+ * @param className the class name
+ * @param cl the class loader
+ * @return is the class name loadable with the class loader
+ */
+ def isClassLoadable(
+ className: String,
+ cl: ClassLoader = Thread.currentThread().getContextClassLoader): Boolean =
+ Try {
+ DynClasses.builder().loader(cl).impl(className).buildChecked()
+ }.isSuccess
+
def getFieldVal[T](target: Any, fieldName: String): T =
Try {
DynFields.builder().hiddenImpl(target.getClass, fieldName).build[T]().get(target)
diff --git a/kyuubi-util-scala/src/test/java/org/apache/kyuubi/util/reflect/ReflectUtilsSuite.scala b/kyuubi-util-scala/src/test/java/org/apache/kyuubi/util/reflect/ReflectUtilsSuite.scala
new file mode 100644
index 000000000..f28024f18
--- /dev/null
+++ b/kyuubi-util-scala/src/test/java/org/apache/kyuubi/util/reflect/ReflectUtilsSuite.scala
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kyuubi.util.reflect
+
+import org.apache.kyuubi.util.reflect.ReflectUtils._
+// scalastyle:off
+import org.scalatest.funsuite.AnyFunSuite
+class ReflectUtilsSuite extends AnyFunSuite {
+ // scalastyle:on
+
+ test("check class loadable") {
+ assert(isClassLoadable(getClass.getName))
+ assert(!isClassLoadable("org.apache.kyuubi.NonExistClass"))
+ }
+}