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 f3a621cab [KYUUBI #5765] Avoid array copy in generating process
builder commands
f3a621cab is described below
commit f3a621cab96926d49a8230228097fd5789adf15e
Author: Bowen Liang <[email protected]>
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 <[email protected]>
Signed-off-by: Bowen Liang <[email protected]>
---
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")
}
