[KYUUBI #5765] Avoid array copy in generating process builder commands

# 🔍 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 🔖 - [ ] 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 ⚰️ No behavior change. #### Behavior With This Pull Request 🎉 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 8ece849 [Bowen Liang] update 0702a6a [Bowen Liang] style e390809 [Bowen Liang] fix processBuilder d2d0fe2 [Bowen Liang] use Iterable as data type of process builder's commands Authored-by: Bowen Liang <[email protected]> Signed-off-by: Bowen Liang <[email protected]>
apache · Nov 28, 2023 · f3a621c · f3a621c
1 parent c8b4dd1
commit f3a621c
Show file tree

Hide file tree

Showing 11 changed files with 21 additions and 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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")
 }