You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/04/19 06:53:39 UTC
[GitHub] [incubator-kyuubi] jiaoqingbo opened a new pull request, #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
jiaoqingbo opened a new pull request, #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418
…xecutable
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/contributions.html
2. If the PR is related to an issue in https://github.com/apache/incubator-kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
fix #2346
### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [ ] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861489914
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
+ s"${File.separator}config.sh && $executable")
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ commandStr.append(s" -D$KYUUBI_SESSION_USER_KEY=$proxyUser ")
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ val confStr = conf.getAll.filter { case (k, _) =>
+ k.startsWith("kyuubi.") || k.startsWith("flink.") ||
+ k.startsWith("hadoop.") || k.startsWith("yarn.")
+ }.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
+ commandStr.append(confStr)
+
+ commandStr.append(" -cp ")
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.toLowerCase.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ env.get("HADOOP_CONF_DIR").foreach(classpathEntries.add)
+ env.get("YARN_CONF_DIR").foreach(classpathEntries.add)
+ env.get("HBASE_CONF_DIR").foreach(classpathEntries.add)
+ val hadoopClasspath = env.get("HADOOP_CLASSPATH")
Review Comment:
how about FLINK_HADOOP_CLASSPATH?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861496654
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
Review Comment:
A brief example, assuming you have a test.sh
export TEST=1
bash test.sh && echo $TEST will produce nothing
source test.sh && echo $TEST will produce 1
as @pan3793 suggested above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r852726014
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -40,44 +42,74 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
+ ("FLINK_SQL_ENGINE_JAR" -> mainResource.get)
Review Comment:
> we can remove these
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861502536
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
+ s"${File.separator}config.sh && $executable")
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ commandStr.append(s" -D$KYUUBI_SESSION_USER_KEY=$proxyUser ")
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ val confStr = conf.getAll.filter { case (k, _) =>
+ k.startsWith("kyuubi.") || k.startsWith("flink.") ||
+ k.startsWith("hadoop.") || k.startsWith("yarn.")
+ }.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
+ commandStr.append(confStr)
+
+ commandStr.append(" -cp ")
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.toLowerCase.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ env.get("HADOOP_CONF_DIR").foreach(classpathEntries.add)
+ env.get("YARN_CONF_DIR").foreach(classpathEntries.add)
+ env.get("HBASE_CONF_DIR").foreach(classpathEntries.add)
+ val hadoopClasspath = env.get("HADOOP_CLASSPATH")
Review Comment:
> yes
I think another pr is needed to handle ENGINE_FLINK_MEMORY, ENGINE_FLINK_JAVA_OPTIONS, ENGINE_FLINK_EXTRA_CLASSPATH
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861489368
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
Review Comment:
why do we need source?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r852707407
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -40,44 +42,74 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
+ ("FLINK_SQL_ENGINE_JAR" -> mainResource.get)
Review Comment:
we can remove these
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r852726253
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -40,44 +42,74 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
+ ("FLINK_SQL_ENGINE_JAR" -> mainResource.get)
Review Comment:
> we can remove these
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861491934
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
+ s"${File.separator}config.sh && $executable")
Review Comment:
what does config.sh do? can we clarify it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861507002
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
Review Comment:
In the previous implementation， do NOT let config.sh detect FLINK_HOME，so we need to pass FLINK_HOME。maybe we can remove FLINK_CONF_DIR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861524528
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
Review Comment:
I'm guessing the purpose of this is to configure via Kyuubi rather than take the defaults，
@SteNicholas WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r857045784
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -40,44 +42,71 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
- ("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += executable
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ buffer += s"-D$KYUUBI_SESSION_USER_KEY=$proxyUser"
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ for ((k, v) <- conf.getAll) {
+ buffer += s"-D$k=$v"
+ }
+ buffer += "-cp"
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ var hadoopConfDir = env.get("HADOOP_CONF_DIR")
+ val hadoopClassPath = env.get("HADOOP_CLASSPATH")
+ if (hadoopConfDir.isEmpty && hadoopClassPath.isEmpty) {
+ val defaultHadoopConfDir = s"${File.separator}etc${File.separator}hadoop${File.separator}conf"
Review Comment:
IMO, when the `hadoopConfDir` and `hadoopClassPath` is empty, the operation that the hadoop conf is setting to `/etc/hadoop/confg` at default is confusing for users, because the hadoop conf directory is logged in the ouput of the Flink engine bootstrap procedure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861501289
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
+ s"${File.separator}config.sh && $executable")
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ commandStr.append(s" -D$KYUUBI_SESSION_USER_KEY=$proxyUser ")
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ val confStr = conf.getAll.filter { case (k, _) =>
+ k.startsWith("kyuubi.") || k.startsWith("flink.") ||
+ k.startsWith("hadoop.") || k.startsWith("yarn.")
+ }.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
+ commandStr.append(confStr)
+
+ commandStr.append(" -cp ")
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.toLowerCase.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ env.get("HADOOP_CONF_DIR").foreach(classpathEntries.add)
+ env.get("YARN_CONF_DIR").foreach(classpathEntries.add)
+ env.get("HBASE_CONF_DIR").foreach(classpathEntries.add)
+ val hadoopClasspath = env.get("HADOOP_CLASSPATH")
Review Comment:
yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861490387
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilderSuite.scala:
##########
@@ -17,15 +17,90 @@
package org.apache.kyuubi.engine.flink
-import org.apache.kyuubi.KyuubiFunSuite
+import java.io.File
+
+import scala.collection.JavaConverters._
+import scala.collection.immutable.ListMap
+
+import org.apache.kyuubi.{FLINK_COMPILE_VERSION, KyuubiFunSuite, KyuubiSQLException, SCALA_COMPILE_VERSION}
import org.apache.kyuubi.config.KyuubiConf
class FlinkProcessBuilderSuite extends KyuubiFunSuite {
private def conf = KyuubiConf().set("kyuubi.on", "off")
+ private def envDefault: ListMap[String, String] = ListMap(
+ "JAVA_HOME" -> s"${File.separator}jdk1.8.0_181")
Review Comment:
why hard_coded to 181
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861491179
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilderSuite.scala:
##########
@@ -17,15 +17,90 @@
package org.apache.kyuubi.engine.flink
-import org.apache.kyuubi.KyuubiFunSuite
+import java.io.File
+
+import scala.collection.JavaConverters._
+import scala.collection.immutable.ListMap
+
+import org.apache.kyuubi.{FLINK_COMPILE_VERSION, KyuubiFunSuite, KyuubiSQLException, SCALA_COMPILE_VERSION}
import org.apache.kyuubi.config.KyuubiConf
class FlinkProcessBuilderSuite extends KyuubiFunSuite {
private def conf = KyuubiConf().set("kyuubi.on", "off")
+ private def envDefault: ListMap[String, String] = ListMap(
+ "JAVA_HOME" -> s"${File.separator}jdk1.8.0_181")
Review Comment:
>
Nothing special, just for testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r857052247
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -40,44 +42,71 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
- ("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += executable
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ buffer += s"-D$KYUUBI_SESSION_USER_KEY=$proxyUser"
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ for ((k, v) <- conf.getAll) {
+ buffer += s"-D$k=$v"
+ }
+ buffer += "-cp"
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ var hadoopConfDir = env.get("HADOOP_CONF_DIR")
+ val hadoopClassPath = env.get("HADOOP_CLASSPATH")
+ if (hadoopConfDir.isEmpty && hadoopClassPath.isEmpty) {
+ val defaultHadoopConfDir = s"${File.separator}etc${File.separator}hadoop${File.separator}conf"
Review Comment:
> IMO, when the `hadoopConfDir` and `hadoopClassPath` is empty, the operation that the hadoop conf is setting to `/etc/hadoop/confg` at default is confusing for users, because the hadoop conf directory is logged in the ouput of the Flink engine bootstrap procedure.
ok I will delete this
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilderSuite.scala:
##########
@@ -17,16 +17,105 @@
package org.apache.kyuubi.engine.flink
-import org.apache.kyuubi.KyuubiFunSuite
+import java.io.File
+import java.nio.file.{Files, Paths}
+import java.util.LinkedHashSet
+
+import scala.collection.JavaConverters._
+import scala.collection.mutable.ArrayBuffer
+
+import org.apache.kyuubi.{FLINK_COMPILE_VERSION, KyuubiFunSuite, SCALA_COMPILE_VERSION}
import org.apache.kyuubi.config.KyuubiConf
class FlinkProcessBuilderSuite extends KyuubiFunSuite {
private def conf = KyuubiConf().set("kyuubi.on", "off")
+ private def envDefault: Map[String, String] = Map(
+ "JAVA_HOME" -> s"${File.separator}jdk1.8.0_181")
+ private def confStr: String = {
+ val configBuffer = new ArrayBuffer[String]()
+ for ((k, v) <- conf.getAll) {
+ configBuffer += s"-D$k=$v"
+ }
+ configBuffer.toArray.mkString(" ")
+ }
+ private val javaPath = s"${envDefault("JAVA_HOME")}${File.separator}bin${File.separator}java"
+ private val flinkSqlClientJarPathSuffix = s"${File.separator}opt${File.separator}" +
+ s"flink-sql-client_$SCALA_COMPILE_VERSION-$FLINK_COMPILE_VERSION.jar"
+ private val flinkLibPathSuffix = s"${File.separator}lib${File.separator}*"
+ private val flinkConfPathSuffix = s"${File.separator}conf"
+ private val mainClassStr = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
test("flink engine process builder") {
- val builder = new FlinkProcessBuilder("vinoyang", conf)
- val commands = builder.toString.split(' ')
- assert(commands.exists(_ endsWith "flink-sql-engine.sh"))
+ val builder = new FlinkProcessBuilder("vinoyang", conf) {
+ override protected def env: Map[String, String] = envDefault +
+ ("HADOOP_CONF_DIR" -> s"${File.separator}hadoop${File.separator}conf") +
+ ("HBASE_CONF_DIR" -> s"${File.separator}hbase${File.separator}conf") +
+ ("YARN_CONF_DIR" -> s"${File.separator}yarn${File.separator}conf") +
+ ("HADOOP_CLASSPATH" -> s"${File.separator}hadoop")
+ }
+ val commands = builder.toString
+
+ val classpathEntries = new LinkedHashSet[String]
+ builder.mainResource.foreach(classpathEntries.add)
+ val flinkSqlClientJarPath = s"${builder.FLINK_HOME}$flinkSqlClientJarPathSuffix"
+ val flinkLibPath = s"${builder.FLINK_HOME}$flinkLibPathSuffix"
+ val flinkConfPath = s"${builder.FLINK_HOME}$flinkConfPathSuffix"
+ val hadoopConfPath = s"${File.separator}hadoop${File.separator}conf"
Review Comment:
> Could this be optimized to iterate the above map for env?
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r853652478
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -40,44 +42,74 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
+ ("FLINK_SQL_ENGINE_JAR" -> mainResource.get)
Review Comment:
> we can remove these
May be we should transfer FLINK_HOME and FLINK_CONF_DIR，otherwise UT may failed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r855435370
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilderSuite.scala:
##########
@@ -17,16 +17,31 @@
package org.apache.kyuubi.engine.flink
+import java.io.File
+
import org.apache.kyuubi.KyuubiFunSuite
import org.apache.kyuubi.config.KyuubiConf
class FlinkProcessBuilderSuite extends KyuubiFunSuite {
private def conf = KyuubiConf().set("kyuubi.on", "off")
test("flink engine process builder") {
- val builder = new FlinkProcessBuilder("vinoyang", conf)
- val commands = builder.toString.split(' ')
- assert(commands.exists(_ endsWith "flink-sql-engine.sh"))
+ val builder = new FlinkProcessBuilder("vinoyang", conf) {
+ override protected def env: Map[String, String] = Map(
+ "JAVA_HOME" -> s"${File.separator}jdk1.8.0_181",
+ "HADOOP_CLASSPATH" -> s"${File.separator}hadoop")
Review Comment:
Does this need to add other environment variables like `HADOOP_CONF_DIR` to test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r855699006
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -40,44 +42,72 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
- ("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
+ ("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += executable
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ buffer += s"-D$KYUUBI_SESSION_USER_KEY=$proxyUser"
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ for ((k, v) <- conf.getAll) {
+ buffer += s"-D$k=$v"
+ }
- override protected def commands: Array[String] = Array(executable)
+ buffer += "-cp"
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ env.get("HADOOP_CONF_DIR").foreach(classpathEntries.add)
+ env.get("YARN_CONF_DIR").foreach(classpathEntries.add)
+ env.get("HBASE_CONF_DIR").foreach(classpathEntries.add)
Review Comment:
> Does the `HBASE_CONF_DIR` need to be added into the classpath here?
config.sh added HBASE_CONF_DIR when building INTERNAL_HADOOP_CLASSPATHS, I keep it consistent with the previous one
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r856095633
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/FlinkSQLEngine.scala:
##########
@@ -125,6 +124,20 @@ object FlinkSQLEngine extends Logging {
}
}
+ def executeConfigShell(): Unit = {
+ val flinkHome = System.getenv("FLINK_HOME")
+ require(flinkHome != null, s"FLINK_HOME is null")
+ val commands = Seq("/bin/bash", s"$flinkHome/bin/config.sh")
+ val pb = new ProcessBuilder(commands: _*)
+ val process = pb.start()
Review Comment:
A brief example, assuming you have a `test.sh`
```
export TEST=1
```
`bash test.sh && echo $TEST` will produce nothing
`source test.sh && echo $TEST` will produce `1`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861412490
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
+ s"${File.separator}config.sh && $executable")
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ commandStr.append(s" -D$KYUUBI_SESSION_USER_KEY=$proxyUser ")
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ val confStr = conf.getAll.filter { case (k, _) =>
+ k.startsWith("kyuubi.") || k.startsWith("flink.") ||
+ k.startsWith("hadoop.") || k.startsWith("yarn.")
+ }.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
+ commandStr.append(confStr)
+
+ commandStr.append(" -cp ")
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ env.get("HADOOP_CONF_DIR").foreach(classpathEntries.add)
+ env.get("YARN_CONF_DIR").foreach(classpathEntries.add)
+ env.get("HBASE_CONF_DIR").foreach(classpathEntries.add)
+ val hadoopClasspath = env.get("HADOOP_CLASSPATH")
+ if (hadoopClasspath.isEmpty) {
+ throw KyuubiSQLException("HADOOP_CLASSPATH is not set! " +
+ "For more detail information on installing and configuring Flink, please visit " +
Review Comment:
> The exception message is something confusing for users, which link isn't used to install and configure Flink.
I will change it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#issuecomment-1113149284
thanks, merged to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#issuecomment-1102230060
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2418?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2418](https://codecov.io/gh/apache/incubator-kyuubi/pull/2418?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3aa8738) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/bafc2f84a2584f6e728dd4aad6f7b9e5f2db83ad?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bafc2f8) will **decrease** coverage by `0.00%`.
> The diff coverage is `72.50%`.
```diff
@@ Coverage Diff @@
## master #2418 +/- ##
============================================
- Coverage 62.92% 62.91% -0.01%
Complexity 69 69
============================================
Files 362 362
Lines 17290 17310 +20
Branches 2334 2333 -1
============================================
+ Hits 10879 10890 +11
- Misses 5386 5397 +11
+ Partials 1025 1023 -2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/2418?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...rg/apache/kyuubi/engine/flink/FlinkSQLEngine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2418/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1mbGluay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9mbGluay9GbGlua1NRTEVuZ2luZS5zY2FsYQ==) | `19.51% <0.00%> (-2.41%)` | :arrow_down: |
| [...ache/kyuubi/engine/flink/FlinkProcessBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2418/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvZmxpbmsvRmxpbmtQcm9jZXNzQnVpbGRlci5zY2FsYQ==) | `82.92% <93.54%> (+8.27%)` | :arrow_up: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2418/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `83.33% <0.00%> (-8.34%)` | :arrow_down: |
| [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2418/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL1RyaW5vU3RhdGVtZW50LnNjYWxh) | `68.96% <0.00%> (-2.30%)` | :arrow_down: |
| [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2418/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `94.44% <0.00%> (-1.86%)` | :arrow_down: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2418/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `96.56% <0.00%> (+0.11%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2418?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2418?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [bafc2f8...3aa8738](https://codecov.io/gh/apache/incubator-kyuubi/pull/2418?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r854743335
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilderSuite.scala:
##########
@@ -26,7 +26,10 @@ class FlinkProcessBuilderSuite extends KyuubiFunSuite {
test("flink engine process builder") {
val builder = new FlinkProcessBuilder("vinoyang", conf)
val commands = builder.toString.split(' ')
- assert(commands.exists(_ endsWith "flink-sql-engine.sh"))
+ assert(commands.head.endsWith("bin/java"), "wrong exec")
Review Comment:
> Why not assert the `commands.mkString(" ")`?
commands contain full paths to many jars. Concatenating these strings in UT is complicated. So it is reasonable to keep the original UT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r855431762
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -40,44 +42,72 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
- ("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
+ ("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += executable
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ buffer += s"-D$KYUUBI_SESSION_USER_KEY=$proxyUser"
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ for ((k, v) <- conf.getAll) {
+ buffer += s"-D$k=$v"
+ }
- override protected def commands: Array[String] = Array(executable)
+ buffer += "-cp"
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ env.get("HADOOP_CONF_DIR").foreach(classpathEntries.add)
+ env.get("YARN_CONF_DIR").foreach(classpathEntries.add)
+ env.get("HBASE_CONF_DIR").foreach(classpathEntries.add)
+
+ val hadoopCp = env.get("HADOOP_CLASSPATH")
+ hadoopCp.foreach(path => classpathEntries.add(s"$path${File.separator}*"))
+ if (hadoopCp.isEmpty) {
Review Comment:
Why do you check whether hadoop classpath is empty here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861499142
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
+ s"${File.separator}config.sh && $executable")
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ commandStr.append(s" -D$KYUUBI_SESSION_USER_KEY=$proxyUser ")
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ val confStr = conf.getAll.filter { case (k, _) =>
+ k.startsWith("kyuubi.") || k.startsWith("flink.") ||
+ k.startsWith("hadoop.") || k.startsWith("yarn.")
+ }.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
+ commandStr.append(confStr)
+
+ commandStr.append(" -cp ")
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.toLowerCase.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ env.get("HADOOP_CONF_DIR").foreach(classpathEntries.add)
+ env.get("YARN_CONF_DIR").foreach(classpathEntries.add)
+ env.get("HBASE_CONF_DIR").foreach(classpathEntries.add)
+ val hadoopClasspath = env.get("HADOOP_CLASSPATH")
Review Comment:
> how about FLINK_HADOOP_CLASSPATH?
Do you mean the same as ENGINE_HIVE_EXTRA_CLASSPATH, ENGINE_TRINO_EXTRA_CLASSPATH？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861507002
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
Review Comment:
do NOT let config.sh detect FLINK_HOME，so we need to pass FLINK_HOME。maybe we can remove FLINK_CONF_DIR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861537119
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
+ s"${File.separator}config.sh && $executable")
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ commandStr.append(s" -D$KYUUBI_SESSION_USER_KEY=$proxyUser ")
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ val confStr = conf.getAll.filter { case (k, _) =>
+ k.startsWith("kyuubi.") || k.startsWith("flink.") ||
+ k.startsWith("hadoop.") || k.startsWith("yarn.")
+ }.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
+ commandStr.append(confStr)
+
+ commandStr.append(" -cp ")
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.toLowerCase.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ env.get("HADOOP_CONF_DIR").foreach(classpathEntries.add)
+ env.get("YARN_CONF_DIR").foreach(classpathEntries.add)
+ env.get("HBASE_CONF_DIR").foreach(classpathEntries.add)
Review Comment:
this should be done in the pr which will handle ENGINE_FLINK_MEMORY, ENGINE_FLINK_JAVA_OPTIONS, ENGINE_FLINK_EXTRA_CLASSPATH
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861538304
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
+ s"${File.separator}config.sh && $executable")
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ commandStr.append(s" -D$KYUUBI_SESSION_USER_KEY=$proxyUser ")
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ val confStr = conf.getAll.filter { case (k, _) =>
+ k.startsWith("kyuubi.") || k.startsWith("flink.") ||
+ k.startsWith("hadoop.") || k.startsWith("yarn.")
+ }.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
+ commandStr.append(confStr)
+
+ commandStr.append(" -cp ")
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.toLowerCase.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ env.get("HADOOP_CONF_DIR").foreach(classpathEntries.add)
+ env.get("YARN_CONF_DIR").foreach(classpathEntries.add)
+ env.get("HBASE_CONF_DIR").foreach(classpathEntries.add)
Review Comment:
The purpose of this pr is to convert flink-sql-engine.sh to java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861491255
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
Review Comment:
why these 2 cannot be removed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r852726253
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -40,44 +42,74 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
+ ("FLINK_SQL_ENGINE_JAR" -> mainResource.get)
Review Comment:
> we can remove these
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r855704146
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilderSuite.scala:
##########
@@ -17,16 +17,31 @@
package org.apache.kyuubi.engine.flink
+import java.io.File
+
import org.apache.kyuubi.KyuubiFunSuite
import org.apache.kyuubi.config.KyuubiConf
class FlinkProcessBuilderSuite extends KyuubiFunSuite {
private def conf = KyuubiConf().set("kyuubi.on", "off")
test("flink engine process builder") {
- val builder = new FlinkProcessBuilder("vinoyang", conf)
- val commands = builder.toString.split(' ')
- assert(commands.exists(_ endsWith "flink-sql-engine.sh"))
+ val builder = new FlinkProcessBuilder("vinoyang", conf) {
+ override protected def env: Map[String, String] = Map(
+ "JAVA_HOME" -> s"${File.separator}jdk1.8.0_181",
+ "HADOOP_CLASSPATH" -> s"${File.separator}hadoop")
Review Comment:
>
OK
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861513057
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
Review Comment:
the bin/config.sh will call multiple times? why do NOT let it detect FLINK_HOME, It(the comment) also affects our logic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861492609
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
+ s"${File.separator}config.sh && $executable")
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ commandStr.append(s" -D$KYUUBI_SESSION_USER_KEY=$proxyUser ")
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ val confStr = conf.getAll.filter { case (k, _) =>
Review Comment:
why we do not respect other configuations, dfs.xx, hive.xx, io.xx etc, they are hadoop related configuration @SteNicholas @deadwind4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861524528
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
Review Comment:
I'm guessing the purpose of this is to configure via Kyuubi rather than take the defaults，
@SteNicholas WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861583492
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
Review Comment:
> StringBuilder
I need to use StringBuilder to concatenate the content of the last element of the ArrayBuffer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#issuecomment-1102160407
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r853993673
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilderSuite.scala:
##########
@@ -26,7 +26,10 @@ class FlinkProcessBuilderSuite extends KyuubiFunSuite {
test("flink engine process builder") {
val builder = new FlinkProcessBuilder("vinoyang", conf)
val commands = builder.toString.split(' ')
- assert(commands.exists(_ endsWith "flink-sql-engine.sh"))
+ assert(commands.head.endsWith("bin/java"), "wrong exec")
Review Comment:
Why not assert the `commands.mkString(" ")`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r856090952
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/FlinkSQLEngine.scala:
##########
@@ -125,6 +124,20 @@ object FlinkSQLEngine extends Logging {
}
}
+ def executeConfigShell(): Unit = {
+ val flinkHome = System.getenv("FLINK_HOME")
+ require(flinkHome != null, s"FLINK_HOME is null")
+ val commands = Seq("/bin/bash", s"$flinkHome/bin/config.sh")
+ val pb = new ProcessBuilder(commands: _*)
+ val process = pb.start()
Review Comment:
`bash config.sh` is different from `source config.sh`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861578153
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
Review Comment:
We already know the value of FLINK_HOME when calling config.sh, so there is no need to set the default value for FLINK_HOME through config.sh
@SteNicholas WDYT？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861355024
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
+ s"${File.separator}config.sh && $executable")
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ commandStr.append(s" -D$KYUUBI_SESSION_USER_KEY=$proxyUser ")
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ val confStr = conf.getAll.filter { case (k, _) =>
+ k.startsWith("kyuubi.") || k.startsWith("flink.") ||
+ k.startsWith("hadoop.") || k.startsWith("yarn.")
+ }.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
+ commandStr.append(confStr)
+
+ commandStr.append(" -cp ")
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ env.get("HADOOP_CONF_DIR").foreach(classpathEntries.add)
+ env.get("YARN_CONF_DIR").foreach(classpathEntries.add)
+ env.get("HBASE_CONF_DIR").foreach(classpathEntries.add)
+ val hadoopClasspath = env.get("HADOOP_CLASSPATH")
+ if (hadoopClasspath.isEmpty) {
+ throw KyuubiSQLException("HADOOP_CLASSPATH is not set! " +
+ "For more detail information on installing and configuring Flink, please visit " +
Review Comment:
The exception message is something confusing for users, which link isn't used to install and configure Flink.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861352386
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
+ s"${File.separator}config.sh && $executable")
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ commandStr.append(s" -D$KYUUBI_SESSION_USER_KEY=$proxyUser ")
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ val confStr = conf.getAll.filter { case (k, _) =>
+ k.startsWith("kyuubi.") || k.startsWith("flink.") ||
+ k.startsWith("hadoop.") || k.startsWith("yarn.")
+ }.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
+ commandStr.append(confStr)
+
+ commandStr.append(" -cp ")
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.startsWith("flink-sql-client")
Review Comment:
```suggestion
name.toLowerCase.startsWith("flink-sql-client")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861494240
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
+ s"${File.separator}config.sh && $executable")
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ commandStr.append(s" -D$KYUUBI_SESSION_USER_KEY=$proxyUser ")
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ val confStr = conf.getAll.filter { case (k, _) =>
+ k.startsWith("kyuubi.") || k.startsWith("flink.") ||
+ k.startsWith("hadoop.") || k.startsWith("yarn.")
+ }.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
+ commandStr.append(confStr)
+
+ commandStr.append(" -cp ")
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.toLowerCase.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ env.get("HADOOP_CONF_DIR").foreach(classpathEntries.add)
+ env.get("YARN_CONF_DIR").foreach(classpathEntries.add)
+ env.get("HBASE_CONF_DIR").foreach(classpathEntries.add)
Review Comment:
we can remove it if
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
+
+ commandStr.append(s"source $FLINK_HOME${File.separator}bin" +
+ s"${File.separator}config.sh && $executable")
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ commandStr.append(s" -D$KYUUBI_SESSION_USER_KEY=$proxyUser ")
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ val confStr = conf.getAll.filter { case (k, _) =>
+ k.startsWith("kyuubi.") || k.startsWith("flink.") ||
+ k.startsWith("hadoop.") || k.startsWith("yarn.")
+ }.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
+ commandStr.append(confStr)
+
+ commandStr.append(" -cp ")
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.toLowerCase.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ env.get("HADOOP_CONF_DIR").foreach(classpathEntries.add)
+ env.get("YARN_CONF_DIR").foreach(classpathEntries.add)
+ env.get("HBASE_CONF_DIR").foreach(classpathEntries.add)
Review Comment:
we can remove it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r861493528
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -37,43 +41,75 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
-
- override protected def commands: Array[String] = Array(executable)
+ ("_FLINK_HOME_DETERMINED" -> s"1")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += s"bash"
+ buffer += s"-c"
+ val commandStr = new StringBuilder()
Review Comment:
ArrayBuffer and StringBuilder, only one is needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#issuecomment-1109188247
@jiaoqingbo, the result of the CI is failure. PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn closed pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
yaooqinn closed pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
URL: https://github.com/apache/incubator-kyuubi/pull/2418
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r857045559
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilderSuite.scala:
##########
@@ -17,16 +17,105 @@
package org.apache.kyuubi.engine.flink
-import org.apache.kyuubi.KyuubiFunSuite
+import java.io.File
+import java.nio.file.{Files, Paths}
+import java.util.LinkedHashSet
+
+import scala.collection.JavaConverters._
+import scala.collection.mutable.ArrayBuffer
+
+import org.apache.kyuubi.{FLINK_COMPILE_VERSION, KyuubiFunSuite, SCALA_COMPILE_VERSION}
import org.apache.kyuubi.config.KyuubiConf
class FlinkProcessBuilderSuite extends KyuubiFunSuite {
private def conf = KyuubiConf().set("kyuubi.on", "off")
+ private def envDefault: Map[String, String] = Map(
+ "JAVA_HOME" -> s"${File.separator}jdk1.8.0_181")
+ private def confStr: String = {
+ val configBuffer = new ArrayBuffer[String]()
+ for ((k, v) <- conf.getAll) {
+ configBuffer += s"-D$k=$v"
+ }
+ configBuffer.toArray.mkString(" ")
+ }
+ private val javaPath = s"${envDefault("JAVA_HOME")}${File.separator}bin${File.separator}java"
+ private val flinkSqlClientJarPathSuffix = s"${File.separator}opt${File.separator}" +
+ s"flink-sql-client_$SCALA_COMPILE_VERSION-$FLINK_COMPILE_VERSION.jar"
+ private val flinkLibPathSuffix = s"${File.separator}lib${File.separator}*"
+ private val flinkConfPathSuffix = s"${File.separator}conf"
+ private val mainClassStr = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
test("flink engine process builder") {
- val builder = new FlinkProcessBuilder("vinoyang", conf)
- val commands = builder.toString.split(' ')
- assert(commands.exists(_ endsWith "flink-sql-engine.sh"))
+ val builder = new FlinkProcessBuilder("vinoyang", conf) {
+ override protected def env: Map[String, String] = envDefault +
+ ("HADOOP_CONF_DIR" -> s"${File.separator}hadoop${File.separator}conf") +
+ ("HBASE_CONF_DIR" -> s"${File.separator}hbase${File.separator}conf") +
+ ("YARN_CONF_DIR" -> s"${File.separator}yarn${File.separator}conf") +
+ ("HADOOP_CLASSPATH" -> s"${File.separator}hadoop")
+ }
+ val commands = builder.toString
+
+ val classpathEntries = new LinkedHashSet[String]
+ builder.mainResource.foreach(classpathEntries.add)
+ val flinkSqlClientJarPath = s"${builder.FLINK_HOME}$flinkSqlClientJarPathSuffix"
+ val flinkLibPath = s"${builder.FLINK_HOME}$flinkLibPathSuffix"
+ val flinkConfPath = s"${builder.FLINK_HOME}$flinkConfPathSuffix"
+ val hadoopConfPath = s"${File.separator}hadoop${File.separator}conf"
Review Comment:
Could this be optimized to iterate the above map for env?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#issuecomment-1102218007
cc @yaooqinn @SteNicholas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r855433072
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -40,44 +42,72 @@ class FlinkProcessBuilder(
val extraEngineLog: Option[OperationLog] = None)
extends ProcBuilder with Logging {
- override protected def executable: String = {
- val flinkEngineHomeOpt = env.get("FLINK_ENGINE_HOME").orElse {
- val cwd = Utils.getCodeSourceLocation(getClass)
- .split("kyuubi-server")
- assert(cwd.length > 1)
- Option(
- Paths.get(cwd.head)
- .resolve("externals")
- .resolve("kyuubi-flink-sql-engine")
- .toFile)
- .map(_.getAbsolutePath)
- }
-
- flinkEngineHomeOpt.map { dir =>
- Paths.get(dir, "bin", FLINK_ENGINE_BINARY_FILE).toAbsolutePath.toFile.getCanonicalPath
- } getOrElse {
- throw KyuubiSQLException("FLINK_ENGINE_HOME is not set! " +
- "For more detail information on installing and configuring Flink, please visit " +
- "https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments")
- }
- }
-
override protected def module: String = "kyuubi-flink-sql-engine"
override protected def mainClass: String = "org.apache.kyuubi.engine.flink.FlinkSQLEngine"
override protected def childProcEnv: Map[String, String] = conf.getEnvs +
("FLINK_HOME" -> FLINK_HOME) +
- ("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf") +
- ("FLINK_SQL_ENGINE_JAR" -> mainResource.get) +
- ("FLINK_SQL_ENGINE_DYNAMIC_ARGS" ->
- conf.getAll.filter { case (k, _) =>
- k.startsWith("kyuubi.") || k.startsWith("flink.") ||
- k.startsWith("hadoop.") || k.startsWith("yarn.")
- }.map { case (k, v) => s"-D$k=$v" }.mkString(" "))
+ ("FLINK_CONF_DIR" -> s"$FLINK_HOME/conf")
+
+ override protected def commands: Array[String] = {
+ val buffer = new ArrayBuffer[String]()
+ buffer += executable
+
+ // TODO: How shall we deal with proxyUser,
+ // user.name
+ // kyuubi.session.user
+ // or just leave it, because we can handle it at operation layer
+ buffer += s"-D$KYUUBI_SESSION_USER_KEY=$proxyUser"
+
+ // TODO: add Kyuubi.engineEnv.FLINK_ENGINE_MEMORY or kyuubi.engine.flink.memory to configure
+ // -Xmx5g
+ // java options
+ for ((k, v) <- conf.getAll) {
+ buffer += s"-D$k=$v"
+ }
- override protected def commands: Array[String] = Array(executable)
+ buffer += "-cp"
+ val classpathEntries = new LinkedHashSet[String]
+ // flink engine runtime jar
+ mainResource.foreach(classpathEntries.add)
+ // flink sql client jar
+ val flinkSqlClientPath = Paths.get(FLINK_HOME)
+ .resolve("opt")
+ .toFile
+ .listFiles(new FilenameFilter {
+ override def accept(dir: File, name: String): Boolean = {
+ name.startsWith("flink-sql-client")
+ }
+ }).head.getAbsolutePath
+ classpathEntries.add(flinkSqlClientPath)
+
+ // jars from flink lib
+ classpathEntries.add(s"$FLINK_HOME${File.separator}lib${File.separator}*")
+
+ // classpath contains flink configurations, default to flink.home/conf
+ classpathEntries.add(env.getOrElse("FLINK_CONF_DIR", s"$FLINK_HOME${File.separator}conf"))
+ // classpath contains hadoop configurations
+ env.get("HADOOP_CONF_DIR").foreach(classpathEntries.add)
+ env.get("YARN_CONF_DIR").foreach(classpathEntries.add)
+ env.get("HBASE_CONF_DIR").foreach(classpathEntries.add)
Review Comment:
Does the `HBASE_CONF_DIR` need to be added into the classpath here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r854743335
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilderSuite.scala:
##########
@@ -26,7 +26,10 @@ class FlinkProcessBuilderSuite extends KyuubiFunSuite {
test("flink engine process builder") {
val builder = new FlinkProcessBuilder("vinoyang", conf)
val commands = builder.toString.split(' ')
- assert(commands.exists(_ endsWith "flink-sql-engine.sh"))
+ assert(commands.head.endsWith("bin/java"), "wrong exec")
Review Comment:
> Why not assert the `commands.mkString(" ")`?
commands contain full paths to many jars. Concatenating these strings in UT is complicated. So it is reasonable to keep the original UT
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilderSuite.scala:
##########
@@ -26,7 +26,10 @@ class FlinkProcessBuilderSuite extends KyuubiFunSuite {
test("flink engine process builder") {
val builder = new FlinkProcessBuilder("vinoyang", conf)
val commands = builder.toString.split(' ')
- assert(commands.exists(_ endsWith "flink-sql-engine.sh"))
+ assert(commands.head.endsWith("bin/java"), "wrong exec")
Review Comment:
> Why not assert the `commands.mkString(" ")`?
OK
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] SteNicholas commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r853989693
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/FlinkSQLEngine.scala:
##########
@@ -125,6 +124,19 @@ object FlinkSQLEngine extends Logging {
}
}
+ def executeConfigShell(): Unit = {
+ val flinkHome = System.getenv("FLINK_HOME")
+ val commands = Seq("/bin/bash", s"$flinkHome/bin/config.sh")
Review Comment:
Please add the check whether the `FLINK_HOME` is null.
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilderSuite.scala:
##########
@@ -26,7 +26,10 @@ class FlinkProcessBuilderSuite extends KyuubiFunSuite {
test("flink engine process builder") {
val builder = new FlinkProcessBuilder("vinoyang", conf)
val commands = builder.toString.split(' ')
- assert(commands.exists(_ endsWith "flink-sql-engine.sh"))
+ assert(commands.head.endsWith("bin/java"), "wrong exec")
Review Comment:
Why not directly assert the `commands.mkString(" ")`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #2418: [KYUUBI #2346] [Improvement] Simplify FlinkProcessBuilder with java executable
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2418:
URL: https://github.com/apache/incubator-kyuubi/pull/2418#discussion_r856095633
##########
externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/FlinkSQLEngine.scala:
##########
@@ -125,6 +124,20 @@ object FlinkSQLEngine extends Logging {
}
}
+ def executeConfigShell(): Unit = {
+ val flinkHome = System.getenv("FLINK_HOME")
+ require(flinkHome != null, s"FLINK_HOME is null")
+ val commands = Seq("/bin/bash", s"$flinkHome/bin/config.sh")
+ val pb = new ProcessBuilder(commands: _*)
+ val process = pb.start()
Review Comment:
A brief example, assuming you have a `test.sh`
```
export TEST=1
```
And `bash test.sh && echo $TEST` will produce nothing, but `source test.sh && echo $TEST` will produce `1`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org