This is an automated email from the ASF dual-hosted git repository. bowenliang pushed a commit to branch branch-1.8 in repository https://gitbox.apache.org/repos/asf/kyuubi.git
The following commit(s) were added to refs/heads/branch-1.8 by this push: new 7aa309fe6 [KYUUBI #5765] Avoid array copy in generating process builder commands 7aa309fe6 is described below commit 7aa309fe652887161c2fe6169d995f4c47fc81fb Author: Bowen Liang <liangbo...@gf.com.cn> AuthorDate: Tue Nov 28 11:14:19 2023 +0800 [KYUUBI #5765] Avoid array copy in generating process builder commands # :mag: Description ## Issue References ๐ As described. ## Describe Your Solution ๐ง Currently, each process builder use `ArrayBuffer.toArray` for generating process builder commands which includes unnecessary array copy. Reuse the buffer and use `Iteratable` for commands. ## Types of changes :bookmark: - [ ] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan ๐งช #### Behavior Without This Pull Request :coffin: No behavior change. #### Behavior With This Pull Request :tada: No behavior change. #### Related Unit Tests No behavior change. --- # Checklists ## ๐ Author Self Checklist - [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project - [x] I have performed a self-review - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) ## ๐ Committer Pre-Merge Checklist - [ ] Pull request title is okay. - [ ] No license issues. - [ ] Milestone correctly set? - [ ] Test coverage is ok - [ ] Assignees are selected. - [ ] Minimum number of approvals - [ ] No changes are requested **Be nice. Be informative.** Closes #5765 from bowenliang123/commands-iter. Closes #5765 8ece849e7 [Bowen Liang] update 0702a6a0b [Bowen Liang] style e3908091b [Bowen Liang] fix processBuilder d2d0fe26e [Bowen Liang] use Iterable as data type of process builder's commands Authored-by: Bowen Liang <liangbo...@gf.com.cn> Signed-off-by: Bowen Liang <liangbo...@gf.com.cn> (cherry picked from commit f3a621cab96926d49a8230228097fd5789adf15e) Signed-off-by: liangbowen <liangbo...@gf.com.cn> --- kyuubi-common/src/main/scala/org/apache/kyuubi/Utils.scala | 2 +- kyuubi-common/src/test/scala/org/apache/kyuubi/UtilsSuite.scala | 4 ++-- .../src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala | 4 ++-- .../scala/org/apache/kyuubi/engine/chat/ChatProcessBuilder.scala | 4 ++-- .../scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala | 7 +++---- .../scala/org/apache/kyuubi/engine/hive/HiveProcessBuilder.scala | 4 ++-- .../scala/org/apache/kyuubi/engine/jdbc/JdbcProcessBuilder.scala | 4 ++-- .../org/apache/kyuubi/engine/spark/SparkBatchProcessBuilder.scala | 4 ++-- .../scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala | 4 ++-- .../scala/org/apache/kyuubi/engine/trino/TrinoProcessBuilder.scala | 4 ++-- .../org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala | 2 +- 11 files changed, 21 insertions(+), 22 deletions(-) diff --git a/kyuubi-common/src/main/scala/org/apache/kyuubi/Utils.scala b/kyuubi-common/src/main/scala/org/apache/kyuubi/Utils.scala index accfca4c9..0144dadbb 100644 --- a/kyuubi-common/src/main/scala/org/apache/kyuubi/Utils.scala +++ b/kyuubi-common/src/main/scala/org/apache/kyuubi/Utils.scala @@ -340,7 +340,7 @@ object Utils extends Logging { private val PATTERN_FOR_KEY_VALUE_ARG = "(.+?)=(.+)".r - def redactCommandLineArgs(conf: KyuubiConf, commands: Array[String]): Array[String] = { + def redactCommandLineArgs(conf: KyuubiConf, commands: Iterable[String]): Iterable[String] = { val redactionPattern = conf.get(SERVER_SECRET_REDACTION_PATTERN) var nextKV = false commands.map { diff --git a/kyuubi-common/src/test/scala/org/apache/kyuubi/UtilsSuite.scala b/kyuubi-common/src/test/scala/org/apache/kyuubi/UtilsSuite.scala index 5973fc6e7..97d9cd1b5 100644 --- a/kyuubi-common/src/test/scala/org/apache/kyuubi/UtilsSuite.scala +++ b/kyuubi-common/src/test/scala/org/apache/kyuubi/UtilsSuite.scala @@ -167,7 +167,7 @@ class UtilsSuite extends KyuubiFunSuite { buffer += "--conf" buffer += "kyuubi.regular.property2=regular_value" - val commands = buffer.toArray + val commands = buffer // Redact sensitive information val redactedCmdArgs = Utils.redactCommandLineArgs(conf, commands) @@ -183,7 +183,7 @@ class UtilsSuite extends KyuubiFunSuite { expectBuffer += "--conf" expectBuffer += "kyuubi.regular.property2=regular_value" - assert(expectBuffer.toArray === redactedCmdArgs) + assert(expectBuffer === redactedCmdArgs) } test("redact sensitive information") { diff --git a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala index 9c7f0f510..23196bf1d 100644 --- a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala +++ b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala @@ -99,7 +99,7 @@ trait ProcBuilder { protected def proxyUser: String - protected val commands: Array[String] + protected val commands: Iterable[String] def conf: KyuubiConf @@ -142,7 +142,7 @@ trait ProcBuilder { } final lazy val processBuilder: ProcessBuilder = { - val pb = new ProcessBuilder(commands: _*) + val pb = new ProcessBuilder(commands.toStream.asJava) val envs = pb.environment() envs.putAll(env.asJava) diff --git a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/chat/ChatProcessBuilder.scala b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/chat/ChatProcessBuilder.scala index 121a20f5f..ade6026b1 100644 --- a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/chat/ChatProcessBuilder.scala +++ b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/chat/ChatProcessBuilder.scala @@ -59,7 +59,7 @@ class ChatProcessBuilder( */ override protected def mainClass: String = "org.apache.kyuubi.engine.chat.ChatEngine" - override protected val commands: Array[String] = { + override protected val commands: Iterable[String] = { val buffer = new ArrayBuffer[String]() buffer += executable @@ -98,7 +98,7 @@ class ChatProcessBuilder( buffer += "--conf" buffer += s"$k=$v" } - buffer.toArray + buffer } override def toString: String = { diff --git a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala index f43adfbc2..52364f189 100644 --- a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala +++ b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala @@ -77,7 +77,7 @@ class FlinkProcessBuilder( ApplicationManagerInfo(clusterManager()) } - override protected val commands: Array[String] = { + override protected val commands: Iterable[String] = { KyuubiApplicationManager.tagApplication(engineRefId, shortName, clusterManager(), conf) // unset engine credentials because Flink doesn't support them at the moment conf.unset(KyuubiReservedKeys.KYUUBI_ENGINE_CREDENTIALS_KEY) @@ -142,8 +142,7 @@ class FlinkProcessBuilder( buffer += s"$k=$v" } } - - buffer.toArray + buffer case _ => val buffer = new ArrayBuffer[String]() @@ -211,7 +210,7 @@ class FlinkProcessBuilder( buffer += "--conf" buffer += s"$k=$v" } - buffer.toArray + buffer } } diff --git a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/hive/HiveProcessBuilder.scala b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/hive/HiveProcessBuilder.scala index 3325d5f2e..d7e270911 100644 --- a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/hive/HiveProcessBuilder.scala +++ b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/hive/HiveProcessBuilder.scala @@ -52,7 +52,7 @@ class HiveProcessBuilder( override protected def mainClass: String = "org.apache.kyuubi.engine.hive.HiveSQLEngine" - override protected val commands: Array[String] = { + override protected val commands: Iterable[String] = { KyuubiApplicationManager.tagApplication(engineRefId, shortName, clusterManager(), conf) val buffer = new ArrayBuffer[String]() buffer += executable @@ -113,7 +113,7 @@ class HiveProcessBuilder( buffer += "--conf" buffer += s"$k=$v" } - buffer.toArray + buffer } override def shortName: String = "hive" diff --git a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/jdbc/JdbcProcessBuilder.scala b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/jdbc/JdbcProcessBuilder.scala index 3849c6431..5b52dbbb4 100644 --- a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/jdbc/JdbcProcessBuilder.scala +++ b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/jdbc/JdbcProcessBuilder.scala @@ -59,7 +59,7 @@ class JdbcProcessBuilder( */ override protected def mainClass: String = "org.apache.kyuubi.engine.jdbc.JdbcSQLEngine" - override protected val commands: Array[String] = { + override protected val commands: Iterable[String] = { require( conf.get(ENGINE_JDBC_CONNECTION_URL).nonEmpty, s"Jdbc server url can not be null! Please set ${ENGINE_JDBC_CONNECTION_URL.key}") @@ -101,7 +101,7 @@ class JdbcProcessBuilder( buffer += "--conf" buffer += s"$k=$v" } - buffer.toArray + buffer } override def toString: String = { diff --git a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkBatchProcessBuilder.scala b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkBatchProcessBuilder.scala index ef159bb93..7d69b90d5 100644 --- a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkBatchProcessBuilder.scala +++ b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkBatchProcessBuilder.scala @@ -36,7 +36,7 @@ class SparkBatchProcessBuilder( extends SparkProcessBuilder(proxyUser, conf, batchId, extraEngineLog) { import SparkProcessBuilder._ - override protected lazy val commands: Array[String] = { + override protected lazy val commands: Iterable[String] = { val buffer = new ArrayBuffer[String]() buffer += executable Option(mainClass).foreach { cla => @@ -66,7 +66,7 @@ class SparkBatchProcessBuilder( batchArgs.foreach { arg => buffer += arg } - buffer.toArray + buffer } private def sparkAppNameConf(): Map[String, String] = { diff --git a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala index 086ca057d..57d5f7335 100644 --- a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala +++ b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala @@ -122,7 +122,7 @@ class SparkProcessBuilder( file.isDirectory && r.findFirstMatchIn(file.getName).isDefined } - override protected lazy val commands: Array[String] = { + override protected lazy val commands: Iterable[String] = { // complete `spark.master` if absent on kubernetes completeMasterUrl(conf) @@ -149,7 +149,7 @@ class SparkProcessBuilder( mainResource.foreach { r => buffer += r } - buffer.toArray + buffer } override protected def module: String = "kyuubi-spark-sql-engine" diff --git a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/trino/TrinoProcessBuilder.scala b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/trino/TrinoProcessBuilder.scala index 5b755dec5..04dc49e03 100644 --- a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/trino/TrinoProcessBuilder.scala +++ b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/trino/TrinoProcessBuilder.scala @@ -50,7 +50,7 @@ class TrinoProcessBuilder( override protected def mainClass: String = "org.apache.kyuubi.engine.trino.TrinoSqlEngine" - override protected val commands: Array[String] = { + override protected val commands: Iterable[String] = { KyuubiApplicationManager.tagApplication(engineRefId, shortName, clusterManager(), conf) require( conf.get(ENGINE_TRINO_CONNECTION_URL).nonEmpty, @@ -104,7 +104,7 @@ class TrinoProcessBuilder( buffer += "--conf" buffer += s"$k=$v" } - buffer.toArray + buffer } override def shortName: String = "trino" diff --git a/kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala b/kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala index 7bbe4ad06..27fd36815 100644 --- a/kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala +++ b/kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala @@ -427,5 +427,5 @@ class SparkProcessBuilderSuite extends KerberizedTestHelper with MockitoSugar { class FakeSparkProcessBuilder(config: KyuubiConf) extends SparkProcessBuilder("fake", config) { - override protected lazy val commands: Array[String] = Array("ls") + override protected lazy val commands: Iterable[String] = Seq("ls") }