Skip to content

Commit

Permalink
[KYUUBI #5761] Pretty ProcBuilder generated command
Browse files Browse the repository at this point in the history
# 🔍 Description
## Issue References 🔗

This PR aims to pretty the generated commands by `ProcBuilder`.

## Describe Your Solution 🔧

Optimized the format rules.

## Types of changes 🔖

- [x] Improvement (non-breaking change)
- [ ] 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 ⚰️

Example with generated Hive engine bootstrap command:
```
00:44:48.326 KyuubiSessionManager-exec-pool: Thread-42 INFO EngineRef: Launching engine:
/home/chengpan/.sdkman/candidates/java/current/bin/java
-Xmx1g
-cp
/home/chengpan/Projects/apache-kyuubi/externals/kyuubi-hive-sql-engine/target/kyuubi-hive-sql-engine_2.12-1.9.0-SNAPSHOT.jar:/home/chengpan/app/apache-hive-2.3.9-bin/conf:/home/chengpan/app/apache-hive-2.3.9-bin/lib/*:/home/chengpan/Projects/apa
che-kyuubi/externals/kyuubi-hive-sql-engine/target/scala-2.12/jars/*
org.apache.kyuubi.engine.hive.HiveSQLEngine
--conf
kyuubi.session.user=chengpanproxy
--conf
kyuubi.engine.id=991b68ac-11fe-4e0e-a043-7c8c8e1fb761
--conf
hadoop.proxyuser.chengpan.groups=*
--conf
hadoop.proxyuser.chengpan.hosts=*
--conf
hive.engine.name=kyuubi_CONNECTION_HIVE_SQL_chengpanproxy_991b68ac-11fe-4e0e-a043-7c8c8e1fb761
--conf
hive.server2.proxy.user=chengpanproxy
--conf
hive.server2.thrift.resultset.default.fetch.size=1000
--conf
javax.jdo.option.ConnectionURL=jdbc:derby:;databaseName=/home/chengpan/Projects/apache-kyuubi/integration-tests/kyuubi-hive-it/target/tmp/KyuubiOperationHiveEnginePerConnectionSuite-67853d06-09da-4c39-8a3c-41315119a1f7;create=true
--conf
kyuubi.client.ipAddress=127.0.1.1
--conf
kyuubi.client.version=1.9.0-SNAPSHOT
--conf
kyuubi.engine.credentials=SERUUwAAAA==
--conf
kyuubi.engine.operation.log.dir.root=target/engine_operation_logs
--conf
kyuubi.engine.share.level=connection
--conf
kyuubi.engine.submit.time=1700757888303
--conf
kyuubi.engine.type=HIVE_SQL
--conf
kyuubi.engineEnv.KYUUBI_HOME=/home/chengpan/Projects/apache-kyuubi/
--conf
kyuubi.frontend.protocols=THRIFT_BINARY
--conf
kyuubi.ha.addresses=10.221.99.150:42613
--conf
kyuubi.ha.engine.ref.id=991b68ac-11fe-4e0e-a043-7c8c8e1fb761
--conf
kyuubi.ha.namespace=/kyuubi_1.9.0-SNAPSHOT_CONNECTION_HIVE_SQL/chengpanproxy/991b68ac-11fe-4e0e-a043-7c8c8e1fb761
--conf
kyuubi.ha.zookeeper.auth.type=NONE
--conf
kyuubi.ha.zookeeper.connection.retry.policy=ONE_TIME
--conf
kyuubi.metrics.json.location=/home/chengpan/Projects/apache-kyuubi/integration-tests/kyuubi-hive-it/target/metrics
--conf
kyuubi.server.ipAddress=127.0.0.1
--conf
kyuubi.session.connection.url=localhost:38775
--conf
kyuubi.session.engine.check.interval=PT1S
--conf
kyuubi.session.engine.idle.timeout=PT30S
--conf
kyuubi.session.real.user=chengpan
--conf
kyuubi.zookeeper.embedded.client.port=0
--conf
kyuubi.zookeeper.embedded.data.dir=/home/chengpan/Projects/apache-kyuubi/integration-tests/kyuubi-hive-it/target/tmp/kyuubi-b343702d-5e9a-49e8-81db-4c7b6b2da270
--conf
spark.sql.catalogImplementation=in-memory
--conf
spark.ui.enabled=false
```

#### Behavior With This Pull Request 🎉

Example with generated Hive engine bootstrap command:
```
00:33:08.584 KyuubiSessionManager-exec-pool: Thread-42 INFO EngineRef: Launching engine:
/home/chengpan/.sdkman/candidates/java/current/bin/java \
        -Xmx1g \
        -cp /home/chengpan/Projects/apache-kyuubi/externals/kyuubi-hive-sql-engine/target/kyuubi-hive-sql-engine_2.12-1.9.0-SNAPSHOT.jar:/home/chengpan/app/apache-hive-2.3.9-bin/conf:/home/chengpan/app/apache-hive-2.3.9-bin/lib/*:/home/chengpan/Projects/apache-kyuubi/externals/kyuubi-hive-sql-engine/target/scala-2.12/jars/* org.apache.kyuubi.engine.hive.HiveSQLEngine \
        --conf kyuubi.session.user=chengpanproxy \
        --conf kyuubi.engine.id=4c4e090a-04fc-479c-9e95-b2484c073eff \
        --conf hadoop.proxyuser.chengpan.groups=* \
        --conf hadoop.proxyuser.chengpan.hosts=* \
        --conf hive.engine.name=kyuubi_CONNECTION_HIVE_SQL_chengpanproxy_4c4e090a-04fc-479c-9e95-b2484c073eff \
        --conf hive.server2.proxy.user=chengpanproxy \
        --conf hive.server2.thrift.resultset.default.fetch.size=1000 \
        --conf javax.jdo.option.ConnectionURL=jdbc:derby:;databaseName=/home/chengpan/Projects/apache-kyuubi/integration-tests/kyuubi-hive-it/target/tmp/KyuubiOperationHiveEnginePerConnectionSuite-a2c9b027-42a5-4dc3-a2ce-021bc7046dda;create=true \
        --conf kyuubi.client.ipAddress=127.0.1.1 \
        --conf kyuubi.client.version=1.9.0-SNAPSHOT \
        --conf kyuubi.engine.credentials=SERUUwAAAA== \
        --conf kyuubi.engine.operation.log.dir.root=target/engine_operation_logs \
        --conf kyuubi.engine.share.level=connection \
        --conf kyuubi.engine.submit.time=1700757188556 \
        --conf kyuubi.engine.type=HIVE_SQL \
        --conf kyuubi.engineEnv.KYUUBI_HOME=/home/chengpan/Projects/apache-kyuubi/ \
        --conf kyuubi.frontend.protocols=THRIFT_BINARY \
        --conf kyuubi.ha.addresses=10.221.99.150:38083 \
        --conf kyuubi.ha.engine.ref.id=4c4e090a-04fc-479c-9e95-b2484c073eff \
        --conf kyuubi.ha.namespace=/kyuubi_1.9.0-SNAPSHOT_CONNECTION_HIVE_SQL/chengpanproxy/4c4e090a-04fc-479c-9e95-b2484c073eff \
        --conf kyuubi.ha.zookeeper.auth.type=NONE \
        --conf kyuubi.ha.zookeeper.connection.retry.policy=ONE_TIME \
        --conf kyuubi.metrics.json.location=/home/chengpan/Projects/apache-kyuubi/integration-tests/kyuubi-hive-it/target/metrics \
        --conf kyuubi.server.ipAddress=127.0.0.1 \
        --conf kyuubi.session.connection.url=localhost:38545 \
        --conf kyuubi.session.engine.check.interval=PT1S \
        --conf kyuubi.session.engine.idle.timeout=PT30S \
        --conf kyuubi.session.real.user=chengpan \
        --conf kyuubi.zookeeper.embedded.client.port=0 \
        --conf kyuubi.zookeeper.embedded.data.dir=/home/chengpan/Projects/apache-kyuubi/integration-tests/kyuubi-hive-it/target/tmp/kyuubi-1f99fdd6-c042-4377-a320-b056139cada1 \
        --conf spark.sql.catalogImplementation=in-memory \
        --conf spark.ui.enabled=false
```

#### Related Unit Tests

See code diff.

---

# 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
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [x] 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 #5761 from pan3793/pretty-cmd.

Closes #5761

e8d830b [Cheng Pan] Pretty ProcBuilder generated command

Authored-by: Cheng Pan <[email protected]>
Signed-off-by: Cheng Pan <[email protected]>
  • Loading branch information
pan3793 committed Nov 24, 2023
1 parent f7bbe5b commit 4d48796
Show file tree
Hide file tree
Showing 9 changed files with 68 additions and 58 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -287,10 +287,10 @@ trait ProcBuilder {

override def toString: String = {
if (commands == null) {
super.toString()
super.toString
} else {
Utils.redactCommandLineArgs(conf, commands).map {
case arg if arg.startsWith("--") => s"\\\n\t$arg"
case arg if arg.startsWith("-") => s"\\\n\t$arg"
case arg => arg
}.mkString(" ")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,15 @@ class ChatProcessBuilder(

override def toString: String = {
if (commands == null) {
super.toString()
super.toString
} else {
Utils.redactCommandLineArgs(conf, commands).map {
case arg if arg.startsWith("-") || arg == mainClass => s"\\\n\t$arg"
case arg if arg.contains(ENGINE_CHAT_GPT_API_KEY.key) =>
s"${ENGINE_CHAT_GPT_API_KEY.key}=$REDACTION_REPLACEMENT_TEXT"
case arg => arg
}.map {
case arg if arg.startsWith("-") || arg == mainClass => s"\\\n\t$arg"
case arg => arg
}.mkString(" ")
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,6 @@ class HiveProcessBuilder(
buffer.toArray
}

override def toString: String = Utils.redactCommandLineArgs(conf, commands).mkString("\n")

override def shortName: String = "hive"
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,13 +106,16 @@ class JdbcProcessBuilder(

override def toString: String = {
if (commands == null) {
super.toString()
super.toString
} else {
Utils.redactCommandLineArgs(conf, commands).map {
case arg if arg.contains(ENGINE_JDBC_CONNECTION_PASSWORD.key) =>
s"${ENGINE_JDBC_CONNECTION_PASSWORD.key}=$REDACTION_REPLACEMENT_TEXT"
case arg => arg
}.mkString("\n")
}.map {
case arg if arg.startsWith("-") => s"\\\n\t$arg"
case arg => arg
}.mkString(" ")
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ class TrinoProcessBuilder(

override def toString: String = {
if (commands == null) {
super.toString()
super.toString
} else {
Utils.redactCommandLineArgs(conf, commands).map {
case arg if arg.contains(ENGINE_TRINO_CONNECTION_PASSWORD.key) =>
Expand All @@ -121,7 +121,10 @@ class TrinoProcessBuilder(
case arg if arg.contains(ENGINE_TRINO_CONNECTION_TRUSTSTORE_PASSWORD.key) =>
s"${ENGINE_TRINO_CONNECTION_TRUSTSTORE_PASSWORD.key}=$REDACTION_REPLACEMENT_TEXT"
case arg => arg
}.mkString("\n")
}.map {
case arg if arg.startsWith("-") => s"\\\n\t$arg"
case arg => arg
}.mkString(" ")
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -81,28 +81,32 @@ class FlinkProcessBuilderSuite extends KyuubiFunSuite {
val actualCommands = builder.toString
val classpathStr = constructClasspathStr(builder)
val expectedCommands =
s"$javaPath -Xmx512m -agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005 " +
s"-cp $classpathStr $mainClassStr \\\\\\n\\t--conf kyuubi.session.user=vinoyang $confStr"
s"""$javaPath \\\\
|\\t-Xmx512m \\\\
|\\t-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005 \\\\
|\\t-cp $classpathStr $mainClassStr \\\\
|\\t--conf kyuubi.session.user=vinoyang $confStr""".stripMargin
val regex = new Regex(expectedCommands)
val matcher = regex.pattern.matcher(actualCommands)
assert(matcher.matches())
}

private def matchActualAndExpectedApplicationMode(builder: FlinkProcessBuilder): Unit = {
val actualCommands = builder.toString
// scalastyle:off line.size.limit
val expectedCommands =
escapePaths(s"${builder.flinkExecutable} run-application ") +
s"-t yarn-application " +
s"-Dyarn.ship-files=.*\\/flink-sql-client.*jar;.*\\/flink-sql-gateway.*jar;$tempUdfJar" +
s";.*\\/hive-site\\.xml " +
s"-Dyarn\\.application\\.name=kyuubi_.* " +
s"-Dyarn\\.tags=KYUUBI " +
s"-Dcontainerized\\.master\\.env\\.FLINK_CONF_DIR=\\. " +
s"-Dcontainerized\\.master\\.env\\.HIVE_CONF_DIR=\\. " +
s"-Dexecution.target=yarn-application " +
s"-c org\\.apache\\.kyuubi\\.engine\\.flink\\.FlinkSQLEngine " +
s".*kyuubi-flink-sql-engine_.*jar" +
s"(?: \\\\\\n\\t--conf \\S+=\\S+)+"
escapePaths(
s"""${builder.flinkExecutable} run-application \\\\
|\\t-t yarn-application \\\\
|\\t-Dyarn.ship-files=.*flink-sql-client.*jar;.*flink-sql-gateway.*jar;$tempUdfJar;.*hive-site.xml \\\\
|\\t-Dyarn.application.name=kyuubi_.* \\\\
|\\t-Dyarn.tags=KYUUBI \\\\
|\\t-Dcontainerized.master.env.FLINK_CONF_DIR=. \\\\
|\\t-Dcontainerized.master.env.HIVE_CONF_DIR=. \\\\
|\\t-Dexecution.target=yarn-application \\\\
|\\t-c org.apache.kyuubi.engine.flink.FlinkSQLEngine .*kyuubi-flink-sql-engine_.*jar""".stripMargin +
"(?: \\\\\\n\\t--conf \\S+=\\S+)+")
// scalastyle:on line.size.limit
val regex = new Regex(expectedCommands)
val matcher = regex.pattern.matcher(actualCommands)
assert(matcher.matches())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,27 +30,27 @@ class HiveProcessBuilderSuite extends KyuubiFunSuite {
override def env: Map[String, String] = super.env + (HIVE_HADOOP_CLASSPATH_KEY -> "/hadoop")
}
val commands = builder.toString.split('\n')
assert(commands.head.endsWith("bin/java"), "wrong exec")
assert(builder.toString.contains("--conf\nkyuubi.session.user=kyuubi"))
assert(commands.head.contains("bin/java"), "wrong exec")
assert(builder.toString.contains("--conf kyuubi.session.user=kyuubi"))
assert(commands.exists(ss => ss.contains("kyuubi-hive-sql-engine")), "wrong classpath")
assert(builder.toString.contains("--conf\nkyuubi.on=off"))
assert(builder.toString.contains("--conf kyuubi.on=off"))
}

test("default engine memory") {
val conf = KyuubiConf()
.set(ENGINE_HIVE_EXTRA_CLASSPATH, "/hadoop")
val builder = new HiveProcessBuilder("kyuubi", conf)
val commands = builder.toString.split('\n')
assert(commands.contains("-Xmx1g"))
val command = builder.toString
assert(command.contains("-Xmx1g"))
}

test("set engine memory") {
val conf = KyuubiConf()
.set(ENGINE_HIVE_MEMORY, "5g")
.set(ENGINE_HIVE_EXTRA_CLASSPATH, "/hadoop")
val builder = new HiveProcessBuilder("kyuubi", conf)
val commands = builder.toString.split('\n')
assert(commands.contains("-Xmx5g"))
val command = builder.toString
assert(command.contains("-Xmx5g"))
}

test("set engine java opts") {
Expand All @@ -60,8 +60,8 @@ class HiveProcessBuilderSuite extends KyuubiFunSuite {
ENGINE_HIVE_JAVA_OPTIONS,
"-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005")
val builder = new HiveProcessBuilder("kyuubi", conf)
val commands = builder.toString.split('\n')
assert(commands.contains("-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005"))
val command = builder.toString
assert(command.contains("-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005"))
}

test("set engine extra classpath") {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ class JdbcProcessBuilderSuite extends KyuubiFunSuite {
.set(ENGINE_JDBC_CONNECTION_URL.key, "")
.set(ENGINE_JDBC_CONNECTION_PASSWORD.key, "123456")
val builder = new JdbcProcessBuilder("kyuubi", conf)
val commands = builder.toString.split("\n")
assert(commands.head.endsWith("bin/java"), "wrong exec")
assert(builder.toString.contains("--conf\nkyuubi.session.user=kyuubi"))
assert(commands.exists(ss => ss.contains("kyuubi-jdbc-engine")), "wrong classpath")
assert(builder.toString.contains("--conf\nkyuubi.on=off"))
assert(builder.toString.contains(
"--conf\nkyuubi.engine.jdbc.connection.password=*********(redacted)"))
val command = builder.toString
assert(command.contains("bin/java"), "wrong exec")
assert(command.contains("--conf kyuubi.session.user=kyuubi"))
assert(command.contains("kyuubi-jdbc-engine"), "wrong classpath")
assert(command.contains("--conf kyuubi.on=off"))
assert(command.contains(
"--conf kyuubi.engine.jdbc.connection.password=*********(redacted)"))
}

test("capture error from jdbc process builder") {
Expand All @@ -47,17 +47,17 @@ class JdbcProcessBuilderSuite extends KyuubiFunSuite {
val conf = KyuubiConf()
.set(ENGINE_JDBC_CONNECTION_URL.key, "")
val builder = new JdbcProcessBuilder("kyuubi", conf)
val commands = builder.toString.split("\n")
assert(commands.contains("-Xmx1g"))
val command = builder.toString
assert(command.contains("-Xmx1g"))
}

test("set engine memory") {
val conf = KyuubiConf()
.set(ENGINE_JDBC_MEMORY, "5g")
.set(ENGINE_JDBC_CONNECTION_URL.key, "")
val builder = new JdbcProcessBuilder("kyuubi", conf)
val commands = builder.toString.split("\n")
assert(commands.contains("-Xmx5g"))
val command = builder.toString
assert(command.contains("-Xmx5g"))
}

test("set engine java options") {
Expand All @@ -67,16 +67,16 @@ class JdbcProcessBuilderSuite extends KyuubiFunSuite {
ENGINE_JDBC_JAVA_OPTIONS,
"-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005")
val builder = new JdbcProcessBuilder("kyuubi", conf)
val commands = builder.toString.split("\n")
assert(commands.contains("-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005"))
val command = builder.toString
assert(command.contains("-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005"))
}

test("set extra classpath") {
val conf = KyuubiConf()
.set(ENGINE_JDBC_CONNECTION_URL.key, "")
.set(ENGINE_JDBC_EXTRA_CLASSPATH, "/dummy_classpath/*")
val builder = new JdbcProcessBuilder("kyuubi", conf)
val commands = builder.toString
assert(commands.contains("/dummy_classpath/*"))
val command = builder.toString
assert(command.contains("/dummy_classpath/*"))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ class TrinoProcessBuilderSuite extends KyuubiFunSuite {
.set(ENGINE_TRINO_CONNECTION_CATALOG, "dummy_catalog")
val builder = new TrinoProcessBuilder("kyuubi", conf)
val commands = builder.toString.split("\n")
assert(commands.head.endsWith("java"))
assert(builder.toString.contains(s"--conf\n${KYUUBI_SESSION_USER_KEY}=kyuubi"))
assert(builder.toString.contains(s"--conf\n${ENGINE_TRINO_CONNECTION_URL.key}=dummy_url"))
assert(commands.head.contains("java"))
assert(builder.toString.contains(s"--conf ${KYUUBI_SESSION_USER_KEY}=kyuubi"))
assert(builder.toString.contains(s"--conf ${ENGINE_TRINO_CONNECTION_URL.key}=dummy_url"))
assert(builder.toString.contains(
s"--conf\n${ENGINE_TRINO_CONNECTION_CATALOG.key}=dummy_catalog"))
s"--conf ${ENGINE_TRINO_CONNECTION_CATALOG.key}=dummy_catalog"))
}

test("capture error from trino process builder") {
Expand All @@ -49,8 +49,8 @@ class TrinoProcessBuilderSuite extends KyuubiFunSuite {
.set(ENGINE_TRINO_CONNECTION_URL, "dummy_url")
.set(ENGINE_TRINO_CONNECTION_CATALOG, "dummy_catalog")
val builder = new TrinoProcessBuilder("kyuubi", conf)
val commands = builder.toString.split("\n")
assert(commands.contains("-Xmx1g"))
val command = builder.toString
assert(command.contains("-Xmx1g"))
}

test("set engine memory") {
Expand All @@ -59,8 +59,8 @@ class TrinoProcessBuilderSuite extends KyuubiFunSuite {
.set(ENGINE_TRINO_CONNECTION_CATALOG, "dummy_catalog")
.set(ENGINE_TRINO_MEMORY, "5g")
val builder = new TrinoProcessBuilder("kyuubi", conf)
val commands = builder.toString.split("\n")
assert(commands.contains("-Xmx5g"))
val command = builder.toString
assert(command.contains("-Xmx5g"))
}

test("set engine java options") {
Expand All @@ -71,8 +71,8 @@ class TrinoProcessBuilderSuite extends KyuubiFunSuite {
ENGINE_TRINO_JAVA_OPTIONS,
"-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005")
val builder = new TrinoProcessBuilder("kyuubi", conf)
val commands = builder.toString.split("\n")
assert(commands.contains("-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005"))
val command = builder.toString
assert(command.contains("-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005"))
}

test("set extra classpath") {
Expand Down

0 comments on commit 4d48796

Please sign in to comment.