Skip to content

Commit 13af6ae

Browse files
bowenliang123bowenliang123
committed
[KYUUBI #5767] Extract common utils for assembling key value pairs with config option prefix in processbuilder
# 🔍 Description ## Issue References 🔗 As described. ## Describe Your Solution 🔧 - Focus on key points for configuration option assembling, instead of repeating manually command configs assembling - Avoid using magic string value "--conf" / "-cp" in each processbuilder - Extract common utils for assembling key value pairs with config option prefix in processbuilder - Use `mutable.ListBuffer` for command assembling - Extract common method for redact config value by key names - NO changes in expected string value for processbuilder command assertions in test suites ## 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 changes. #### Behavior With This Pull Request 🎉 No behavior changes. #### Related Unit Tests Added `CommandUtilsSuite`. --- # 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 - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [x] 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 #5767 from bowenliang123/config-option. Closes #5767 b043888 [liangbowen] use ++ for command configs 16a3c27 [liangbowen] .key bc28500 [liangbowen] use raw literal in test suites ab018cf [Bowen Liang] config option Lead-authored-by: liangbowen <liangbowen@gf.com.cn> Co-authored-by: Bowen Liang <liangbowen@gf.com.cn> Signed-off-by: liangbowen <liangbowen@gf.com.cn>
1 parent 8f529aa commit 13af6ae

File tree

16 files changed

+248
-167
lines changed

16 files changed

+248
-167
lines changed

‎externals/kyuubi-flink-sql-engine/src/test/scala/org/apache/kyuubi/engine/flink/WithFlinkSQLEngineLocal.scala‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import java.net.URI
2323
import java.nio.file.{Files, Paths}
2424

2525
import scala.collection.JavaConverters._
26-
import scala.collection.mutable.ArrayBuffer
26+
import scala.collection.mutable
2727

2828
import org.apache.flink.configuration.{Configuration, RestOptions}
2929
import org.apache.flink.runtime.minicluster.{MiniCluster, MiniClusterConfiguration}
@@ -32,6 +32,7 @@ import org.apache.kyuubi.{KYUUBI_VERSION, KyuubiException, KyuubiFunSuite, SCALA
3232
import org.apache.kyuubi.config.KyuubiConf
3333
import org.apache.kyuubi.config.KyuubiConf._
3434
import org.apache.kyuubi.ha.HighAvailabilityConf.HA_ADDRESSES
35+
import org.apache.kyuubi.util.command.CommandLineUtils._
3536
import org.apache.kyuubi.zookeeper.EmbeddedZookeeper
3637
import org.apache.kyuubi.zookeeper.ZookeeperConf.{ZK_CLIENT_PORT, ZK_CLIENT_PORT_ADDRESS}
3738

@@ -111,7 +112,7 @@ trait WithFlinkSQLEngineLocal extends KyuubiFunSuite with WithFlinkTestResources
111112
processBuilder.environment().putAll(envs.asJava)
112113

113114
conf.set(ENGINE_FLINK_EXTRA_CLASSPATH, udfJar.getAbsolutePath)
114-
val command = new ArrayBuffer[String]()
115+
val command = new mutable.ListBuffer[String]()
115116

116117
command += envs("JAVA_EXEC")
117118

@@ -122,8 +123,7 @@ trait WithFlinkSQLEngineLocal extends KyuubiFunSuite with WithFlinkTestResources
122123
command += javaOptions.get
123124
}
124125

125-
command += "-cp"
126-
val classpathEntries = new java.util.LinkedHashSet[String]
126+
val classpathEntries = new mutable.LinkedHashSet[String]
127127
// flink engine runtime jar
128128
mainResource(envs).foreach(classpathEntries.add)
129129
// flink sql jars
@@ -163,13 +163,11 @@ trait WithFlinkSQLEngineLocal extends KyuubiFunSuite with WithFlinkTestResources
163163
classpathEntries.add(s"$devHadoopJars${File.separator}*")
164164
}
165165
}
166-
command += classpathEntries.asScala.mkString(File.pathSeparator)
166+
command ++= genClasspathOption(classpathEntries)
167+
167168
command += "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
168169

169-
conf.getAll.foreach { case (k, v) =>
170-
command += "--conf"
171-
command += s"$k=$v"
172-
}
170+
command ++= confKeyValues(conf.getAll)
173171

174172
processBuilder.command(command.toList.asJava)
175173
processBuilder.redirectOutput(Redirect.INHERIT)

‎externals/kyuubi-flink-sql-engine/src/test/scala/org/apache/kyuubi/engine/flink/WithFlinkSQLEngineOnYarn.scala‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import org.apache.kyuubi.{KYUUBI_VERSION, KyuubiFunSuite, SCALA_COMPILE_VERSION,
3434
import org.apache.kyuubi.config.KyuubiConf
3535
import org.apache.kyuubi.config.KyuubiConf.{ENGINE_FLINK_APPLICATION_JARS, KYUUBI_HOME}
3636
import org.apache.kyuubi.ha.HighAvailabilityConf.HA_ADDRESSES
37+
import org.apache.kyuubi.util.command.CommandLineUtils._
3738
import org.apache.kyuubi.zookeeper.EmbeddedZookeeper
3839
import org.apache.kyuubi.zookeeper.ZookeeperConf.{ZK_CLIENT_PORT, ZK_CLIENT_PORT_ADDRESS}
3940

@@ -179,10 +180,7 @@ trait WithFlinkSQLEngineOnYarn extends KyuubiFunSuite with WithFlinkTestResource
179180
conf.set(k, v)
180181
}
181182

182-
for ((k, v) <- conf.getAll) {
183-
command += "--conf"
184-
command += s"$k=$v"
185-
}
183+
command ++= confKeyValues(conf.getAll)
186184

187185
processBuilder.command(command.toList.asJava)
188186
processBuilder.redirectOutput(Redirect.INHERIT)

‎externals/kyuubi-hive-sql-engine/src/test/scala/org/apache/kyuubi/engine/hive/operation/HiveCatalogDatabaseOperationSuite.scala‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,16 +23,17 @@ import org.apache.kyuubi.Utils
2323
import org.apache.kyuubi.config.KyuubiConf.ENGINE_OPERATION_CONVERT_CATALOG_DATABASE_ENABLED
2424
import org.apache.kyuubi.engine.hive.HiveSQLEngine
2525
import org.apache.kyuubi.operation.HiveJDBCTestHelper
26+
import org.apache.kyuubi.util.command.CommandLineUtils._
2627

2728
class HiveCatalogDatabaseOperationSuite extends HiveJDBCTestHelper {
2829

2930
override def beforeAll(): Unit = {
3031
val metastore = Utils.createTempDir(prefix = getClass.getSimpleName)
3132
metastore.toFile.delete()
3233
val args = Array(
33-
"--conf",
34+
CONF,
3435
s"javax.jdo.option.ConnectionURL=jdbc:derby:;databaseName=$metastore;create=true",
35-
"--conf",
36+
CONF,
3637
s"${ENGINE_OPERATION_CONVERT_CATALOG_DATABASE_ENABLED.key}=true")
3738
HiveSQLEngine.main(args)
3839
super.beforeAll()

‎externals/kyuubi-hive-sql-engine/src/test/scala/org/apache/kyuubi/engine/hive/operation/HiveOperationSuite.scala‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,15 @@ import org.apache.commons.lang3.{JavaVersion, SystemUtils}
2222
import org.apache.kyuubi.{HiveEngineTests, KYUUBI_VERSION, Utils}
2323
import org.apache.kyuubi.engine.hive.HiveSQLEngine
2424
import org.apache.kyuubi.jdbc.hive.KyuubiStatement
25+
import org.apache.kyuubi.util.command.CommandLineUtils._
2526

2627
class HiveOperationSuite extends HiveEngineTests {
2728

2829
override def beforeAll(): Unit = {
2930
val metastore = Utils.createTempDir(prefix = getClass.getSimpleName)
3031
metastore.toFile.delete()
3132
val args = Array(
32-
"--conf",
33+
CONF,
3334
s"javax.jdo.option.ConnectionURL=jdbc:derby:;databaseName=$metastore;create=true")
3435
HiveSQLEngine.main(args)
3536
super.beforeAll()

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

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import org.apache.hadoop.util.ShutdownHookManager
4040

4141
import org.apache.kyuubi.config.KyuubiConf
4242
import org.apache.kyuubi.config.internal.Tests.IS_TESTING
43+
import org.apache.kyuubi.util.command.CommandLineUtils._
4344

4445
object Utils extends Logging {
4546

@@ -325,7 +326,7 @@ object Utils extends Logging {
325326
require(args.length % 2 == 0, s"Illegal size of arguments.")
326327
for (i <- args.indices by 2) {
327328
require(
328-
args(i) == "--conf",
329+
args(i) == CONF,
329330
s"Unrecognized main arguments prefix ${args(i)}," +
330331
s"the argument format is '--conf k=v'.")
331332

@@ -336,25 +337,24 @@ object Utils extends Logging {
336337
}
337338
}
338339

339-
val REDACTION_REPLACEMENT_TEXT = "*********(redacted)"
340-
341-
private val PATTERN_FOR_KEY_VALUE_ARG = "(.+?)=(.+)".r
342-
343340
def redactCommandLineArgs(conf: KyuubiConf, commands: Iterable[String]): Iterable[String] = {
344-
val redactionPattern = conf.get(SERVER_SECRET_REDACTION_PATTERN)
345-
var nextKV = false
346-
commands.map {
347-
case PATTERN_FOR_KEY_VALUE_ARG(key, value) if nextKV =>
348-
val (_, newValue) = redact(redactionPattern, Seq((key, value))).head
349-
nextKV = false
350-
s"$key=$newValue"
351-
352-
case cmd if cmd == "--conf" =>
353-
nextKV = true
354-
cmd
355-
356-
case cmd =>
357-
cmd
341+
conf.get(SERVER_SECRET_REDACTION_PATTERN) match {
342+
case Some(redactionPattern) =>
343+
var nextKV = false
344+
commands.map {
345+
case PATTERN_FOR_KEY_VALUE_ARG(key, value) if nextKV =>
346+
val (_, newValue) = redact(redactionPattern, Seq((key, value))).head
347+
nextKV = false
348+
genKeyValuePair(key, newValue)
349+
350+
case cmd if cmd == CONF =>
351+
nextKV = true
352+
cmd
353+
354+
case cmd =>
355+
cmd
356+
}
357+
case _ => commands
358358
}
359359
}
360360

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

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,13 @@ import java.nio.file.{Files, Paths}
2323
import java.security.PrivilegedExceptionAction
2424
import java.util.Properties
2525

26-
import scala.collection.mutable.ArrayBuffer
26+
import scala.collection.mutable
2727

2828
import org.apache.hadoop.security.UserGroupInformation
2929

3030
import org.apache.kyuubi.config.KyuubiConf
3131
import org.apache.kyuubi.config.KyuubiConf.SERVER_SECRET_REDACTION_PATTERN
32+
import org.apache.kyuubi.util.command.CommandLineUtils._
3233

3334
class UtilsSuite extends KyuubiFunSuite {
3435

@@ -156,30 +157,26 @@ class UtilsSuite extends KyuubiFunSuite {
156157
val conf = new KyuubiConf()
157158
conf.set(SERVER_SECRET_REDACTION_PATTERN, "(?i)secret|password".r)
158159

159-
val buffer = new ArrayBuffer[String]()
160+
val buffer = new mutable.ListBuffer[String]()
160161
buffer += "main"
161-
buffer += "--conf"
162-
buffer += "kyuubi.my.password=sensitive_value"
163-
buffer += "--conf"
164-
buffer += "kyuubi.regular.property1=regular_value"
165-
buffer += "--conf"
166-
buffer += "kyuubi.my.secret=sensitive_value"
167-
buffer += "--conf"
168-
buffer += "kyuubi.regular.property2=regular_value"
162+
buffer ++= confKeyValue("kyuubi.my.password", "sensitive_value")
163+
buffer ++= confKeyValue("kyuubi.regular.property1", "regular_value")
164+
buffer ++= confKeyValue("kyuubi.my.secret", "sensitive_value")
165+
buffer ++= confKeyValue("kyuubi.regular.property2", "regular_value")
169166

170167
val commands = buffer
171168

172169
// Redact sensitive information
173170
val redactedCmdArgs = Utils.redactCommandLineArgs(conf, commands)
174171

175-
val expectBuffer = new ArrayBuffer[String]()
172+
val expectBuffer = new mutable.ListBuffer[String]()
176173
expectBuffer += "main"
177174
expectBuffer += "--conf"
178-
expectBuffer += "kyuubi.my.password=" + Utils.REDACTION_REPLACEMENT_TEXT
175+
expectBuffer += "kyuubi.my.password=" + REDACTION_REPLACEMENT_TEXT
179176
expectBuffer += "--conf"
180177
expectBuffer += "kyuubi.regular.property1=regular_value"
181178
expectBuffer += "--conf"
182-
expectBuffer += "kyuubi.my.secret=" + Utils.REDACTION_REPLACEMENT_TEXT
179+
expectBuffer += "kyuubi.my.secret=" + REDACTION_REPLACEMENT_TEXT
183180
expectBuffer += "--conf"
184181
expectBuffer += "kyuubi.regular.property2=regular_value"
185182

@@ -189,11 +186,11 @@ class UtilsSuite extends KyuubiFunSuite {
189186
test("redact sensitive information") {
190187
val secretKeys = Some("my.password".r)
191188
assert(Utils.redact(secretKeys, Seq(("kyuubi.my.password", "12345"))) ===
192-
Seq(("kyuubi.my.password", Utils.REDACTION_REPLACEMENT_TEXT)))
189+
Seq(("kyuubi.my.password", REDACTION_REPLACEMENT_TEXT)))
193190
assert(Utils.redact(secretKeys, Seq(("anything", "kyuubi.my.password=12345"))) ===
194-
Seq(("anything", Utils.REDACTION_REPLACEMENT_TEXT)))
191+
Seq(("anything", REDACTION_REPLACEMENT_TEXT)))
195192
assert(Utils.redact(secretKeys, Seq((999, "kyuubi.my.password=12345"))) ===
196-
Seq((999, Utils.REDACTION_REPLACEMENT_TEXT)))
193+
Seq((999, REDACTION_REPLACEMENT_TEXT)))
197194
// Do not redact when value type is not string
198195
assert(Utils.redact(secretKeys, Seq(("my.password", 12345))) ===
199196
Seq(("my.password", 12345)))

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

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -19,20 +19,18 @@ package org.apache.kyuubi.engine.chat
1919

2020
import java.io.File
2121
import java.nio.file.{Files, Paths}
22-
import java.util
2322

24-
import scala.collection.JavaConverters._
25-
import scala.collection.mutable.ArrayBuffer
23+
import scala.collection.mutable
2624

2725
import com.google.common.annotations.VisibleForTesting
2826

2927
import org.apache.kyuubi.{Logging, SCALA_COMPILE_VERSION, Utils}
30-
import org.apache.kyuubi.Utils.REDACTION_REPLACEMENT_TEXT
3128
import org.apache.kyuubi.config.KyuubiConf
3229
import org.apache.kyuubi.config.KyuubiConf._
3330
import org.apache.kyuubi.config.KyuubiReservedKeys.KYUUBI_SESSION_USER_KEY
3431
import org.apache.kyuubi.engine.ProcBuilder
3532
import org.apache.kyuubi.operation.log.OperationLog
33+
import org.apache.kyuubi.util.command.CommandLineUtils._
3634

3735
class ChatProcessBuilder(
3836
override val proxyUser: String,
@@ -60,7 +58,7 @@ class ChatProcessBuilder(
6058
override protected def mainClass: String = "org.apache.kyuubi.engine.chat.ChatEngine"
6159

6260
override protected val commands: Iterable[String] = {
63-
val buffer = new ArrayBuffer[String]()
61+
val buffer = new mutable.ListBuffer[String]()
6462
buffer += executable
6563

6664
val memory = conf.get(ENGINE_CHAT_MEMORY)
@@ -69,8 +67,7 @@ class ChatProcessBuilder(
6967
val javaOptions = conf.get(ENGINE_CHAT_JAVA_OPTIONS)
7068
javaOptions.foreach(buffer += _)
7169

72-
buffer += "-cp"
73-
val classpathEntries = new util.LinkedHashSet[String]
70+
val classpathEntries = new mutable.LinkedHashSet[String]
7471
mainResource.foreach(classpathEntries.add)
7572
mainResource.foreach { path =>
7673
val parent = Paths.get(path).getParent
@@ -88,28 +85,24 @@ class ChatProcessBuilder(
8885

8986
val extraCp = conf.get(ENGINE_CHAT_EXTRA_CLASSPATH)
9087
extraCp.foreach(classpathEntries.add)
91-
buffer += classpathEntries.asScala.mkString(File.pathSeparator)
88+
buffer ++= genClasspathOption(classpathEntries)
89+
9290
buffer += mainClass
9391

94-
buffer += "--conf"
95-
buffer += s"$KYUUBI_SESSION_USER_KEY=$proxyUser"
92+
buffer ++= confKeyValue(KYUUBI_SESSION_USER_KEY, proxyUser)
93+
94+
buffer ++= confKeyValues(conf.getAll)
9695

97-
conf.getAll.foreach { case (k, v) =>
98-
buffer += "--conf"
99-
buffer += s"$k=$v"
100-
}
10196
buffer
10297
}
10398

10499
override def toString: String = {
105100
if (commands == null) {
106101
super.toString
107102
} else {
108-
Utils.redactCommandLineArgs(conf, commands).map {
109-
case arg if arg.contains(ENGINE_CHAT_GPT_API_KEY.key) =>
110-
s"${ENGINE_CHAT_GPT_API_KEY.key}=$REDACTION_REPLACEMENT_TEXT"
111-
case arg => arg
112-
}.map {
103+
redactConfValues(
104+
Utils.redactCommandLineArgs(conf, commands),
105+
Set(ENGINE_CHAT_GPT_API_KEY.key)).map {
113106
case arg if arg.startsWith("-") || arg == mainClass => s"\\\n\t$arg"
114107
case arg => arg
115108
}.mkString(" ")

0 commit comments

Comments
 (0)