Skip to content

Avoid array copy in generating process builder commands #5765

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Avoid array copy in generating process builder commands #5765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d2d0fe2
use Iterable as data type of process builder's commands
bowenliang123 Nov 24, 2023
e390809
fix processBuilder
bowenliang123 Nov 24, 2023
0702a6a
style
bowenliang123 Nov 24, 2023
8ece849
update
bowenliang123 Nov 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion kyuubi-common/src/main/scala/org/apache/kyuubi/Utils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions kyuubi-common/src/test/scala/org/apache/kyuubi/UtilsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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") {
Expand Down
4 changes: 2 additions & 2 deletions kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ trait ProcBuilder {

protected def proxyUser: String

protected val commands: Array[String]
protected val commands: Iterable[String]

def conf: KyuubiConf

Expand Down Expand Up @@ -142,7 +142,7 @@ trait ProcBuilder {
}

final lazy val processBuilder: ProcessBuilder = {
val pb = new ProcessBuilder(commands: _*)
val pb = new ProcessBuilder(commands.toStream.asJava)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this introduce the overhead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toStream uses a iterator, which prevents array copy to a list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it requires to be converted to Array eventually, so early transformation should be good too. would prefer to keep as-is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I got your point. Closing this PR.


val envs = pb.environment()
envs.putAll(env.asJava)
Expand Down
4 changes: 2 additions & 2 deletions kyuubi-server/src/main/scala/org/apache/kyuubi/engine/chat/ChatProcessBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -98,7 +98,7 @@ class ChatProcessBuilder(
buffer += "--conf"
buffer += s"$k=$v"
}
buffer.toArray
buffer
}

override def toString: String = {
Expand Down
7 changes: 3 additions & 4 deletions kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -142,8 +142,7 @@ class FlinkProcessBuilder(
buffer += s"$k=$v"
}
}

buffer.toArray
buffer

case _ =>
val buffer = new ArrayBuffer[String]()
Expand Down Expand Up @@ -211,7 +210,7 @@ class FlinkProcessBuilder(
buffer += "--conf"
buffer += s"$k=$v"
}
buffer.toArray
buffer
}
}

Expand Down
4 changes: 2 additions & 2 deletions kyuubi-server/src/main/scala/org/apache/kyuubi/engine/hive/HiveProcessBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -113,7 +113,7 @@ class HiveProcessBuilder(
buffer += "--conf"
buffer += s"$k=$v"
}
buffer.toArray
buffer
}

override def shortName: String = "hive"
Expand Down
4 changes: 2 additions & 2 deletions kyuubi-server/src/main/scala/org/apache/kyuubi/engine/jdbc/JdbcProcessBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
Expand Down Expand Up @@ -101,7 +101,7 @@ class JdbcProcessBuilder(
buffer += "--conf"
buffer += s"$k=$v"
}
buffer.toArray
buffer
}

override def toString: String = {
Expand Down
4 changes: 2 additions & 2 deletions kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkBatchProcessBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>
Expand Down Expand Up @@ -66,7 +66,7 @@ class SparkBatchProcessBuilder(

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

buffer.toArray
buffer
}

private def sparkAppNameConf(): Map[String, String] = {
Expand Down
4 changes: 2 additions & 2 deletions kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -149,7 +149,7 @@ class SparkProcessBuilder(

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

buffer.toArray
buffer
}

override protected def module: String = "kyuubi-spark-sql-engine"
Expand Down
4 changes: 2 additions & 2 deletions kyuubi-server/src/main/scala/org/apache/kyuubi/engine/trino/TrinoProcessBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -104,7 +104,7 @@ class TrinoProcessBuilder(
buffer += "--conf"
buffer += s"$k=$v"
}
buffer.toArray
buffer
}

override def shortName: String = "trino"
Expand Down
2 changes: 1 addition & 1 deletion kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}