Skip to content

Commit f3a621c

Browse files
committed
[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 <liangbowen@gf.com.cn> Signed-off-by: Bowen Liang <liangbowen@gf.com.cn>
1 parent c8b4dd1 commit f3a621c

File tree

11 files changed

+21
-22
lines changed

11 files changed

+21
-22
lines changed

kyuubi-common/src/main/scala/org/apache/kyuubi/Utils.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ object Utils extends Logging {
340340

341341
private val PATTERN_FOR_KEY_VALUE_ARG = "(.+?)=(.+)".r
342342

343-
def redactCommandLineArgs(conf: KyuubiConf, commands: Array[String]): Array[String] = {
343+
def redactCommandLineArgs(conf: KyuubiConf, commands: Iterable[String]): Iterable[String] = {
344344
val redactionPattern = conf.get(SERVER_SECRET_REDACTION_PATTERN)
345345
var nextKV = false
346346
commands.map {

kyuubi-common/src/test/scala/org/apache/kyuubi/UtilsSuite.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ class UtilsSuite extends KyuubiFunSuite {
167167
buffer += "--conf"
168168
buffer += "kyuubi.regular.property2=regular_value"
169169

170-
val commands = buffer.toArray
170+
val commands = buffer
171171

172172
// Redact sensitive information
173173
val redactedCmdArgs = Utils.redactCommandLineArgs(conf, commands)
@@ -183,7 +183,7 @@ class UtilsSuite extends KyuubiFunSuite {
183183
expectBuffer += "--conf"
184184
expectBuffer += "kyuubi.regular.property2=regular_value"
185185

186-
assert(expectBuffer.toArray === redactedCmdArgs)
186+
assert(expectBuffer === redactedCmdArgs)
187187
}
188188

189189
test("redact sensitive information") {

kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ trait ProcBuilder {
9999

100100
protected def proxyUser: String
101101

102-
protected val commands: Array[String]
102+
protected val commands: Iterable[String]
103103

104104
def conf: KyuubiConf
105105

@@ -142,7 +142,7 @@ trait ProcBuilder {
142142
}
143143

144144
final lazy val processBuilder: ProcessBuilder = {
145-
val pb = new ProcessBuilder(commands: _*)
145+
val pb = new ProcessBuilder(commands.toStream.asJava)
146146

147147
val envs = pb.environment()
148148
envs.putAll(env.asJava)

kyuubi-server/src/main/scala/org/apache/kyuubi/engine/chat/ChatProcessBuilder.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ class ChatProcessBuilder(
5959
*/
6060
override protected def mainClass: String = "org.apache.kyuubi.engine.chat.ChatEngine"
6161

62-
override protected val commands: Array[String] = {
62+
override protected val commands: Iterable[String] = {
6363
val buffer = new ArrayBuffer[String]()
6464
buffer += executable
6565

@@ -98,7 +98,7 @@ class ChatProcessBuilder(
9898
buffer += "--conf"
9999
buffer += s"$k=$v"
100100
}
101-
buffer.toArray
101+
buffer
102102
}
103103

104104
override def toString: String = {

kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ class FlinkProcessBuilder(
7777
ApplicationManagerInfo(clusterManager())
7878
}
7979

80-
override protected val commands: Array[String] = {
80+
override protected val commands: Iterable[String] = {
8181
KyuubiApplicationManager.tagApplication(engineRefId, shortName, clusterManager(), conf)
8282
// unset engine credentials because Flink doesn't support them at the moment
8383
conf.unset(KyuubiReservedKeys.KYUUBI_ENGINE_CREDENTIALS_KEY)
@@ -142,8 +142,7 @@ class FlinkProcessBuilder(
142142
buffer += s"$k=$v"
143143
}
144144
}
145-
146-
buffer.toArray
145+
buffer
147146

148147
case _ =>
149148
val buffer = new ArrayBuffer[String]()
@@ -211,7 +210,7 @@ class FlinkProcessBuilder(
211210
buffer += "--conf"
212211
buffer += s"$k=$v"
213212
}
214-
buffer.toArray
213+
buffer
215214
}
216215
}
217216

kyuubi-server/src/main/scala/org/apache/kyuubi/engine/hive/HiveProcessBuilder.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ class HiveProcessBuilder(
5252

5353
override protected def mainClass: String = "org.apache.kyuubi.engine.hive.HiveSQLEngine"
5454

55-
override protected val commands: Array[String] = {
55+
override protected val commands: Iterable[String] = {
5656
KyuubiApplicationManager.tagApplication(engineRefId, shortName, clusterManager(), conf)
5757
val buffer = new ArrayBuffer[String]()
5858
buffer += executable
@@ -113,7 +113,7 @@ class HiveProcessBuilder(
113113
buffer += "--conf"
114114
buffer += s"$k=$v"
115115
}
116-
buffer.toArray
116+
buffer
117117
}
118118

119119
override def shortName: String = "hive"

kyuubi-server/src/main/scala/org/apache/kyuubi/engine/jdbc/JdbcProcessBuilder.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ class JdbcProcessBuilder(
5959
*/
6060
override protected def mainClass: String = "org.apache.kyuubi.engine.jdbc.JdbcSQLEngine"
6161

62-
override protected val commands: Array[String] = {
62+
override protected val commands: Iterable[String] = {
6363
require(
6464
conf.get(ENGINE_JDBC_CONNECTION_URL).nonEmpty,
6565
s"Jdbc server url can not be null! Please set ${ENGINE_JDBC_CONNECTION_URL.key}")
@@ -101,7 +101,7 @@ class JdbcProcessBuilder(
101101
buffer += "--conf"
102102
buffer += s"$k=$v"
103103
}
104-
buffer.toArray
104+
buffer
105105
}
106106

107107
override def toString: String = {

kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkBatchProcessBuilder.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ class SparkBatchProcessBuilder(
3636
extends SparkProcessBuilder(proxyUser, conf, batchId, extraEngineLog) {
3737
import SparkProcessBuilder._
3838

39-
override protected lazy val commands: Array[String] = {
39+
override protected lazy val commands: Iterable[String] = {
4040
val buffer = new ArrayBuffer[String]()
4141
buffer += executable
4242
Option(mainClass).foreach { cla =>
@@ -66,7 +66,7 @@ class SparkBatchProcessBuilder(
6666

6767
batchArgs.foreach { arg => buffer += arg }
6868

69-
buffer.toArray
69+
buffer
7070
}
7171

7272
private def sparkAppNameConf(): Map[String, String] = {

kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ class SparkProcessBuilder(
122122
file.isDirectory && r.findFirstMatchIn(file.getName).isDefined
123123
}
124124

125-
override protected lazy val commands: Array[String] = {
125+
override protected lazy val commands: Iterable[String] = {
126126
// complete `spark.master` if absent on kubernetes
127127
completeMasterUrl(conf)
128128

@@ -149,7 +149,7 @@ class SparkProcessBuilder(
149149

150150
mainResource.foreach { r => buffer += r }
151151

152-
buffer.toArray
152+
buffer
153153
}
154154

155155
override protected def module: String = "kyuubi-spark-sql-engine"

kyuubi-server/src/main/scala/org/apache/kyuubi/engine/trino/TrinoProcessBuilder.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ class TrinoProcessBuilder(
5050

5151
override protected def mainClass: String = "org.apache.kyuubi.engine.trino.TrinoSqlEngine"
5252

53-
override protected val commands: Array[String] = {
53+
override protected val commands: Iterable[String] = {
5454
KyuubiApplicationManager.tagApplication(engineRefId, shortName, clusterManager(), conf)
5555
require(
5656
conf.get(ENGINE_TRINO_CONNECTION_URL).nonEmpty,
@@ -104,7 +104,7 @@ class TrinoProcessBuilder(
104104
buffer += "--conf"
105105
buffer += s"$k=$v"
106106
}
107-
buffer.toArray
107+
buffer
108108
}
109109

110110
override def shortName: String = "trino"

0 commit comments

Comments
 (0)