You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "davidyuan1223 (via GitHub)" <gi...@apache.org> on 2023/10/12 10:46:27 UTC
[PR] [Issues #4186] Improvement--add job log [kyuubi]
davidyuan1223 opened a new pull request, #5410:
URL: https://github.com/apache/kyuubi/pull/5410
<!--
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/CONTRIBUTING.html
2. If the PR is related to an issue in https://github.com/apache/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.
-->
### _How was this patch tested?_
Add some test cases that check the changes thoroughly including negative and positive cases if possible
**add unit-test to test show progress with jobInfo**
Add screenshots for manual tests if appropriate
**The photo show unit test case**
**The photo show match log**
**The photo is changes need**
- [ ] [Run test](https://kyuubi.readthedocs.io/en/master/contributing/code/testing.html#running-tests) locally before make a pull request
### _Was this patch authored or co-authored using generative AI tooling?_
<!--
If a generative AI tooling has been used in the process of authoring this patch, please include
phrase 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 commented on PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#issuecomment-1763669833
> > @wForget have fix the code style error. i found the `operation listener` unit test error, is my config make the unit test error?
>
> ```
> - operation listener *** FAILED ***
> The code passed to eventually never returned normally. Attempted 190 times over 1.5037484225833333 minutes. Last failure message: logs.exists(((x$2: String) => x$2.contains("started with 2 stages"))) was false.
> ```
>
> @davidyuan1223 It seems to be a related error. Can you debug it locally? I'll take a look at it later too.
i have check this, actually, this code style i think it's easy to fail, because the logs is not continue, but their assert need a not continue logs contains all assert, it's a not safe code style. i think, the assert test is continue, why not the log is a complete log?
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1360164253
##########
externals/kyuubi-spark-sql-engine/src/test/scala/org/apache/spark/kyuubi/SQLOperationListenerSuite.scala:
##########
@@ -22,16 +22,37 @@ import scala.collection.JavaConverters.asScalaBufferConverter
import org.apache.hive.service.rpc.thrift.{TExecuteStatementReq, TFetchOrientation, TFetchResultsReq, TOperationHandle}
import org.scalatest.time.SpanSugar._
+import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.config.KyuubiConf.OPERATION_SPARK_LISTENER_ENABLED
import org.apache.kyuubi.engine.spark.WithSparkSQLEngine
import org.apache.kyuubi.operation.HiveJDBCTestHelper
class SQLOperationListenerSuite extends WithSparkSQLEngine with HiveJDBCTestHelper {
- override def withKyuubiConf: Map[String, String] = Map.empty
+ override def withKyuubiConf: Map[String, String] = Map(
+ KyuubiConf.ENGINE_SPARK_SHOW_PROGRESS.key -> "true",
Review Comment:
They are only needed by the `operation listener with progress job info` test case, we should move them inside the test case and configure them via `withSessionConf`.
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 commented on PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#issuecomment-1766229806
@cxzl25 looks like my push request has some dirty file changed, please only check the `StageStatus,SparkConsoleProgressBar,SqlOperationListenerSuite` that i real changed'files
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1363360646
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkConsoleProgressBar.scala:
##########
@@ -72,6 +73,17 @@ class SparkConsoleProgressBar(
}
}
+ /**
+ * Use stageId to find stage's jobId
+ * @param stageId stageId
+ * @return jobId,if not exists, return -1
Review Comment:
outdated desc
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1360563887
##########
externals/kyuubi-spark-sql-engine/src/test/scala/org/apache/spark/kyuubi/SQLOperationListenerSuite.scala:
##########
@@ -22,16 +22,37 @@ import scala.collection.JavaConverters.asScalaBufferConverter
import org.apache.hive.service.rpc.thrift.{TExecuteStatementReq, TFetchOrientation, TFetchResultsReq, TOperationHandle}
import org.scalatest.time.SpanSugar._
+import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.config.KyuubiConf.OPERATION_SPARK_LISTENER_ENABLED
import org.apache.kyuubi.engine.spark.WithSparkSQLEngine
import org.apache.kyuubi.operation.HiveJDBCTestHelper
class SQLOperationListenerSuite extends WithSparkSQLEngine with HiveJDBCTestHelper {
- override def withKyuubiConf: Map[String, String] = Map.empty
+ override def withKyuubiConf: Map[String, String] = Map(
+ KyuubiConf.ENGINE_SPARK_SHOW_PROGRESS.key -> "true",
Review Comment:
These two configurations in `SQLOperationListener` are not obtained from spark session conf, we can obtain them like this:
```
private def getSessionConf[T](configEntry: ConfigEntry[T]): T = {
spark.conf.getOption(configEntry.key).map(configEntry.valueConverter).getOrElse {
operation.getSession.sessionManager.getConf.get(configEntry)
}
}
private lazy val consoleProgressBar =
if (getSessionConf(ENGINE_SPARK_SHOW_PROGRESS)) {
Some(new SparkConsoleProgressBar(
operation,
activeStages,
getSessionConf(ENGINE_SPARK_SHOW_PROGRESS_UPDATE_INTERVAL),
getSessionConf(ENGINE_SPARK_SHOW_PROGRESS_TIME_FORMAT)))
} else {
None
}
```
cc @cxzl25 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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1360167379
##########
externals/kyuubi-spark-sql-engine/src/test/scala/org/apache/spark/kyuubi/SQLOperationListenerSuite.scala:
##########
@@ -22,16 +22,37 @@ import scala.collection.JavaConverters.asScalaBufferConverter
import org.apache.hive.service.rpc.thrift.{TExecuteStatementReq, TFetchOrientation, TFetchResultsReq, TOperationHandle}
import org.scalatest.time.SpanSugar._
+import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.config.KyuubiConf.OPERATION_SPARK_LISTENER_ENABLED
import org.apache.kyuubi.engine.spark.WithSparkSQLEngine
import org.apache.kyuubi.operation.HiveJDBCTestHelper
class SQLOperationListenerSuite extends WithSparkSQLEngine with HiveJDBCTestHelper {
- override def withKyuubiConf: Map[String, String] = Map.empty
+ override def withKyuubiConf: Map[String, String] = Map(
+ KyuubiConf.ENGINE_SPARK_SHOW_PROGRESS.key -> "true",
Review Comment:
> They are only needed by the `operation listener with progress job info` test case, we should move them inside the test case and configure them via `withSessionConf`.
get, i will fix 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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#issuecomment-1763637137
> @wForget have fix the code style error. i found the `operation listener` unit test error, is my config make the unit test error?
```
- operation listener *** FAILED ***
The code passed to eventually never returned normally. Attempted 190 times over 1.5037484225833333 minutes. Last failure message: logs.exists(((x$2: String) => x$2.contains("started with 2 stages"))) was false.
```
@davidyuan1223 It seems to be a related error. Can you debug it locally? I'll take a look at it later too.
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 closed pull request #5410: [KYUUBI #4186] Spark showProgress with JobInfo
URL: https://github.com/apache/kyuubi/pull/5410
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1369494931
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkConsoleProgressBar.scala:
##########
@@ -81,7 +93,11 @@ class SparkConsoleProgressBar(
val width = TerminalWidth / stages.size
Review Comment:
i use the debug to test the width
1. this show the lastProgressbar show there only 1 stage, the bar show there have 3 stages
2. our terminalWidth default is 80, i think it's the best width to show progress info, the follow show when we make the default terminalWith is 180, it will show too long
there are 2 advice to improvement this
1. only display 1 stage at the same time
2. the not improvement logic is.
```
// length 9
val header = s"[Stage ${s.stageId}:"
// length 12~15
val tailer = s"(${s.numCompleteTasks} + ${s.numActiveTasks}) / $total]"
// if there have 3 stages, the width is 27. it will show 3~6 precent info
val w = width - header.length - tailer.length
```
we can add jobHeader.length in the expr `val w = width - header.length - tailer.length + jobHeader.lenght`, because we add job Info, so i think it's the best way
just like the photo , the bar show 3 stages at the same time, the lastProgressBar show only 1 stage at the same time
what do you think? @wForget @cxzl25
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1357633767
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/StageStatus.scala:
##########
@@ -27,3 +27,8 @@ class SparkStageInfo(val stageId: Int, val numTasks: Int) {
var numActiveTasks = new AtomicInteger(0)
var numCompleteTasks = new AtomicInteger(0)
}
+
+
+class SparkJobInfo(val numStages: Int, val stageIds: Seq[Int]) {
+ var numCompleteStages = new AtomicInteger(0)
+}
Review Comment:
The file must end with newline, please add it.
check error:
```
[INFO] --- scalastyle-maven-plugin:1.0.0:check (default-cli) @ kyuubi-spark-sql-engine_2.12 ---
error file=/home/runner/work/kyuubi/kyuubi/externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/StageStatus.scala message=File must end with newline character
```
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkConsoleProgressBar.scala:
##########
@@ -72,6 +73,22 @@ class SparkConsoleProgressBar(
}
}
+ /**
+ * Use stageId to find stage's jobId
+ * @param stageId stageId
+ * @return jobId,if not exists, return -1
+ */
+ private def findJobId(stageId: Int): Int = {
Review Comment:
return `Option[Int]` then use `isEmpty/isDefined` to determine whether it is empty
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1361450730
##########
externals/kyuubi-spark-sql-engine/src/test/scala/org/apache/spark/kyuubi/SQLOperationListenerSuite.scala:
##########
@@ -22,16 +22,37 @@ import scala.collection.JavaConverters.asScalaBufferConverter
import org.apache.hive.service.rpc.thrift.{TExecuteStatementReq, TFetchOrientation, TFetchResultsReq, TOperationHandle}
import org.scalatest.time.SpanSugar._
+import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.config.KyuubiConf.OPERATION_SPARK_LISTENER_ENABLED
import org.apache.kyuubi.engine.spark.WithSparkSQLEngine
import org.apache.kyuubi.operation.HiveJDBCTestHelper
class SQLOperationListenerSuite extends WithSparkSQLEngine with HiveJDBCTestHelper {
- override def withKyuubiConf: Map[String, String] = Map.empty
+ override def withKyuubiConf: Map[String, String] = Map(
+ KyuubiConf.ENGINE_SPARK_SHOW_PROGRESS.key -> "true",
Review Comment:
i have try the fast sql test locally, but the fast sql looks like will not get the interval make there not have jobInfo log so that test failed, so i use a slow sql to replace, and the local and ci test all success
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1361826510
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkConsoleProgressBar.scala:
##########
@@ -81,7 +93,14 @@ class SparkConsoleProgressBar(
val width = TerminalWidth / stages.size
val bar = stages.map { s =>
val total = s.numTasks
- val header = s"[Stage ${s.stageId}:"
+ val result = findJobId(s.stageId)
+ var jobHeader = s"[There is no job about this stage]"
+ if (result.isDefined) {
+ val jobId = result.get
+ jobHeader = s"[Job $jobId (${liveJobs.get(jobId).numCompleteStages} " +
+ s"/ ${liveJobs.get(jobId).numStages}) Stages] "
+ }
Review Comment:
```suggestion
val jobHeader = findJobId(s.stageId).map(jobId =>
s"[Job $jobId (${liveJobs.get(jobId).numCompleteStages} " +
s"/ ${liveJobs.get(jobId).numStages}) Stages] ").getOrElse(
"[There is no job about this stage] ")
```
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 commented on PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#issuecomment-1761277831
@wForget have fix the code style error. i found the `operation listener` unit test error, is my config make the unit test error?
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1360162001
##########
externals/kyuubi-spark-sql-engine/src/test/scala/org/apache/spark/kyuubi/SQLOperationListenerSuite.scala:
##########
@@ -22,16 +22,37 @@ import scala.collection.JavaConverters.asScalaBufferConverter
import org.apache.hive.service.rpc.thrift.{TExecuteStatementReq, TFetchOrientation, TFetchResultsReq, TOperationHandle}
import org.scalatest.time.SpanSugar._
+import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.config.KyuubiConf.OPERATION_SPARK_LISTENER_ENABLED
import org.apache.kyuubi.engine.spark.WithSparkSQLEngine
import org.apache.kyuubi.operation.HiveJDBCTestHelper
class SQLOperationListenerSuite extends WithSparkSQLEngine with HiveJDBCTestHelper {
- override def withKyuubiConf: Map[String, String] = Map.empty
+ override def withKyuubiConf: Map[String, String] = Map(
+ KyuubiConf.ENGINE_SPARK_SHOW_PROGRESS.key -> "true",
+ KyuubiConf.ENGINE_SPARK_SHOW_PROGRESS_UPDATE_INTERVAL.key -> "200")
override protected def jdbcUrl: String = getJdbcUrl
+ test("operation listener with progress job info") {
Review Comment:
The "operation listener" test case uses initJobId as jobId, so we need to move "operation listener with progress job info" after "operation listener"
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1366564369
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkConsoleProgressBar.scala:
##########
@@ -81,7 +93,11 @@ class SparkConsoleProgressBar(
val width = TerminalWidth / stages.size
Review Comment:
your guess is right, when there have 3 stages at the same time, the log looks not correct, like the screenshot, the first is 3 stages at the same time, the scond is 1 stage, the width from
`val width = TerminalWidth / stages.size`
`private val TerminalWidth = sys.env.getOrElse("COLUMNS", "80").toInt`
i think we can expand the default TerminalWidth
`[There is no job about this stage] `add length is this
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1360160628
##########
externals/kyuubi-spark-sql-engine/src/test/scala/org/apache/spark/kyuubi/SQLOperationListenerSuite.scala:
##########
@@ -22,16 +22,37 @@ import scala.collection.JavaConverters.asScalaBufferConverter
import org.apache.hive.service.rpc.thrift.{TExecuteStatementReq, TFetchOrientation, TFetchResultsReq, TOperationHandle}
import org.scalatest.time.SpanSugar._
+import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.config.KyuubiConf.OPERATION_SPARK_LISTENER_ENABLED
import org.apache.kyuubi.engine.spark.WithSparkSQLEngine
import org.apache.kyuubi.operation.HiveJDBCTestHelper
class SQLOperationListenerSuite extends WithSparkSQLEngine with HiveJDBCTestHelper {
- override def withKyuubiConf: Map[String, String] = Map.empty
+ override def withKyuubiConf: Map[String, String] = Map(
+ KyuubiConf.ENGINE_SPARK_SHOW_PROGRESS.key -> "true",
+ KyuubiConf.ENGINE_SPARK_SHOW_PROGRESS_UPDATE_INTERVAL.key -> "200")
override protected def jdbcUrl: String = getJdbcUrl
+ test("operation listener with progress job info") {
+ val sql = "SELECT java_method('java.lang.Thread', 'sleep', 10000l) FROM range(1, 3, 1, 2);"
Review Comment:
This sql may cause delayed execution and block subsequent SQL. For this test case, we do not need to execute the blocking sql.
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1361452528
##########
externals/kyuubi-spark-sql-engine/src/test/scala/org/apache/spark/kyuubi/SQLOperationListenerSuite.scala:
##########
@@ -22,16 +22,37 @@ import scala.collection.JavaConverters.asScalaBufferConverter
import org.apache.hive.service.rpc.thrift.{TExecuteStatementReq, TFetchOrientation, TFetchResultsReq, TOperationHandle}
import org.scalatest.time.SpanSugar._
+import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.config.KyuubiConf.OPERATION_SPARK_LISTENER_ENABLED
import org.apache.kyuubi.engine.spark.WithSparkSQLEngine
import org.apache.kyuubi.operation.HiveJDBCTestHelper
class SQLOperationListenerSuite extends WithSparkSQLEngine with HiveJDBCTestHelper {
- override def withKyuubiConf: Map[String, String] = Map.empty
+ override def withKyuubiConf: Map[String, String] = Map(
+ KyuubiConf.ENGINE_SPARK_SHOW_PROGRESS.key -> "true",
Review Comment:
> i have try the fast sql test locally, but the fast sql looks like will not get the interval make there not have jobInfo log so that test failed, so i use a slow sql to replace, and the local and ci test all success
Got 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
Re: [PR] [Issues #4186] Improvement--add job log [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 closed pull request #5410: [Issues #4186] Improvement--add job log
URL: https://github.com/apache/kyuubi/pull/5410
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1357013424
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SQLOperationListener.scala:
##########
@@ -79,37 +80,45 @@ class SQLOperationListener(
}
}
- override def onJobStart(jobStart: SparkListenerJobStart): Unit = activeJobs.synchronized {
- if (sameGroupId(jobStart.properties)) {
- val jobId = jobStart.jobId
- val stageSize = jobStart.stageInfos.size
- if (executionId.isEmpty) {
- executionId = Option(jobStart.properties.getProperty(SPARK_SQL_EXECUTION_ID_KEY))
- .map(_.toLong)
- consoleProgressBar
- operation match {
- case executeStatement: ExecuteStatement =>
- executeStatement.setCompiledStateIfNeeded()
- case _ =>
+ override def onJobStart(jobStart: SparkListenerJobStart): Unit = {
+ activeJobs.synchronized {
Review Comment:
`activeJobs` is a thread-safe `ConcurrentHashMap` object, can we eliminate `synchronized`?
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1357024324
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/StageStatus.scala:
##########
@@ -27,3 +27,8 @@ class SparkStageInfo(val stageId: Int, val numTasks: Int) {
var numActiveTasks = new AtomicInteger(0)
var numCompleteTasks = new AtomicInteger(0)
}
+
+
Review Comment:
remove redundant empty line
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1360291463
##########
externals/kyuubi-spark-sql-engine/src/test/scala/org/apache/spark/kyuubi/SQLOperationListenerSuite.scala:
##########
@@ -22,16 +22,37 @@ import scala.collection.JavaConverters.asScalaBufferConverter
import org.apache.hive.service.rpc.thrift.{TExecuteStatementReq, TFetchOrientation, TFetchResultsReq, TOperationHandle}
import org.scalatest.time.SpanSugar._
+import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.config.KyuubiConf.OPERATION_SPARK_LISTENER_ENABLED
import org.apache.kyuubi.engine.spark.WithSparkSQLEngine
import org.apache.kyuubi.operation.HiveJDBCTestHelper
class SQLOperationListenerSuite extends WithSparkSQLEngine with HiveJDBCTestHelper {
- override def withKyuubiConf: Map[String, String] = Map.empty
+ override def withKyuubiConf: Map[String, String] = Map(
+ KyuubiConf.ENGINE_SPARK_SHOW_PROGRESS.key -> "true",
Review Comment:
have set conf in withSessionConf, but it is useless when init SparkConsoleProgressBar,because the conf is not effective, do you know how to solve this problem?
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1366440042
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkConsoleProgressBar.scala:
##########
@@ -81,7 +93,11 @@ class SparkConsoleProgressBar(
val width = TerminalWidth / stages.size
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1361830691
##########
externals/kyuubi-spark-sql-engine/src/test/scala/org/apache/spark/kyuubi/SQLOperationListenerSuite.scala:
##########
@@ -22,16 +22,37 @@ import scala.collection.JavaConverters.asScalaBufferConverter
import org.apache.hive.service.rpc.thrift.{TExecuteStatementReq, TFetchOrientation, TFetchResultsReq, TOperationHandle}
import org.scalatest.time.SpanSugar._
+import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.config.KyuubiConf.OPERATION_SPARK_LISTENER_ENABLED
import org.apache.kyuubi.engine.spark.WithSparkSQLEngine
import org.apache.kyuubi.operation.HiveJDBCTestHelper
class SQLOperationListenerSuite extends WithSparkSQLEngine with HiveJDBCTestHelper {
- override def withKyuubiConf: Map[String, String] = Map.empty
+ override def withKyuubiConf: Map[String, String] = Map(
+ KyuubiConf.ENGINE_SPARK_SHOW_PROGRESS.key -> "true",
Review Comment:
> These two configurations in SQLOperationListener are not obtained from spark session conf, we can obtain them like this:
+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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1361813258
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SQLOperationListener.scala:
##########
@@ -79,9 +82,10 @@ class SQLOperationListener(
}
}
- override def onJobStart(jobStart: SparkListenerJobStart): Unit = activeJobs.synchronized {
+ override def onJobStart(jobStart: SparkListenerJobStart): Unit = {
if (sameGroupId(jobStart.properties)) {
val jobId = jobStart.jobId
+ val stageIds = jobStart.stageInfos.map(_.stageId)
Review Comment:
```suggestion
val stageIds = jobStart.stageInfos.map(_.stageId).toSet
```
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SQLOperationListener.scala:
##########
@@ -134,9 +140,15 @@ class SQLOperationListener(
override def onStageCompleted(stageCompleted: SparkListenerStageCompleted): Unit = {
val stageInfo = stageCompleted.stageInfo
+ val stageId = stageInfo.stageId
val stageAttempt = SparkStageAttempt(stageInfo.stageId, stageInfo.attemptNumber())
activeStages.synchronized {
if (activeStages.remove(stageAttempt) != null) {
+ activeJobs.asScala.foreach(item => {
+ if (item._2.stageIds.contains(stageId)) {
+ item._2.numCompleteStages.getAndIncrement()
Review Comment:
Add one when the stage is successful.
https://github.com/apache/spark/blob/28961a6ce001e0c25c780a39a726fdd825542cee/core/src/main/scala/org/apache/spark/status/AppStatusListener.scala#L827-L836
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/StageStatus.scala:
##########
@@ -27,3 +27,7 @@ class SparkStageInfo(val stageId: Int, val numTasks: Int) {
var numActiveTasks = new AtomicInteger(0)
var numCompleteTasks = new AtomicInteger(0)
}
+
+class SparkJobInfo(val numStages: Int, val stageIds: Seq[Int]) {
+ var numCompleteStages = new AtomicInteger(0)
Review Comment:
```suggestion
class SparkJobInfo(val numStages: Int, val stageIds: Set[Int]) {
val numCompleteStages = new AtomicInteger(0)
```
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/StageStatus.scala:
##########
@@ -27,3 +27,7 @@ class SparkStageInfo(val stageId: Int, val numTasks: Int) {
var numActiveTasks = new AtomicInteger(0)
var numCompleteTasks = new AtomicInteger(0)
Review Comment:
```suggestion
val numActiveTasks = new AtomicInteger(0)
val numCompleteTasks = new AtomicInteger(0)
```
--
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
Re: [PR] [Improvement] spark showProgress with JobInfo #4186 [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#issuecomment-1759402838
@davidyuan1223 thanks for your contribution, it's not necessary to put everything with screenshots, for example, it's clear to the reviewer to check your unit test code via GitHub. And would mind filling in "Why are the changes needed?", generally, for most of the PR, it's clear for the reviewer to check the code to understand "what you changed", but not "why are the changes needed".
In addition, we provided some contributor guidance in https://github.com/apache/kyuubi/issues/5357, please read and fix the code style issue.
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1357014069
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SQLOperationListener.scala:
##########
@@ -79,37 +80,45 @@ class SQLOperationListener(
}
}
- override def onJobStart(jobStart: SparkListenerJobStart): Unit = activeJobs.synchronized {
- if (sameGroupId(jobStart.properties)) {
- val jobId = jobStart.jobId
- val stageSize = jobStart.stageInfos.size
- if (executionId.isEmpty) {
- executionId = Option(jobStart.properties.getProperty(SPARK_SQL_EXECUTION_ID_KEY))
- .map(_.toLong)
- consoleProgressBar
- operation match {
- case executeStatement: ExecuteStatement =>
- executeStatement.setCompiledStateIfNeeded()
- case _ =>
+ override def onJobStart(jobStart: SparkListenerJobStart): Unit = {
+ activeJobs.synchronized {
+ if (sameGroupId(jobStart.properties)) {
+ val jobId = jobStart.jobId
+ val stageIds = jobStart.stageInfos.map(_.stageId)
+ val stageSize = jobStart.stageInfos.size
+ if (executionId.isEmpty) {
+ executionId = Option(jobStart.properties.getProperty(SPARK_SQL_EXECUTION_ID_KEY))
+ .map(_.toLong)
+ consoleProgressBar
+ operation match {
+ case executeStatement: ExecuteStatement =>
+ executeStatement.setCompiledStateIfNeeded()
+ case _ =>
+ }
+ }
+ activeJobs.put(
+ jobId,
+ new SparkJobInfo(stageSize, stageIds)
+ )
+ withOperationLog {
+ info(s"Query [$operationId]: Job $jobId started with $stageSize stages," +
+ s" ${activeJobs.size()} active jobs running")
}
- }
- withOperationLog {
- activeJobs.add(jobId)
- info(s"Query [$operationId]: Job $jobId started with $stageSize stages," +
- s" ${activeJobs.size()} active jobs running")
}
}
}
override def onJobEnd(jobEnd: SparkListenerJobEnd): Unit = activeJobs.synchronized {
Review Comment:
ditto
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1366426309
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkConsoleProgressBar.scala:
##########
@@ -81,7 +93,11 @@ class SparkConsoleProgressBar(
val width = TerminalWidth / stages.size
Review Comment:
Now we only display the progress information of three stages.
`show(now, stages.take(3)) // display at most 3 stages in same time`
After adding Job information, is the existing width enough to display the information?
Can you test three stages running at the same time?
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1371476027
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SQLOperationListener.scala:
##########
@@ -134,9 +140,18 @@ class SQLOperationListener(
override def onStageCompleted(stageCompleted: SparkListenerStageCompleted): Unit = {
val stageInfo = stageCompleted.stageInfo
+ val stageId = stageInfo.stageId
val stageAttempt = SparkStageAttempt(stageInfo.stageId, stageInfo.attemptNumber())
activeStages.synchronized {
if (activeStages.remove(stageAttempt) != null) {
+ stageInfo.getStatusString match {
+ case "succeeded" =>
+ activeJobs.asScala.foreach(item => {
+ if (item._2.stageIds.contains(stageId)) {
+ item._2.numCompleteStages.getAndIncrement()
+ }
+ })
Review Comment:
```suggestion
activeJobs.asScala.foreach { case (_, jobInfo) =>
if (jobInfo.stageIds.contains(stageId)) {
jobInfo.numCompleteStages.getAndIncrement()
}
}
```
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#issuecomment-1779029141
thanks @davidyuan1223 and all reviewers, merged to master/1.8
--
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
Re: [PR] [Issues #4186] Improvement--add job log [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 closed pull request #5410: [Issues #4186] Improvement--add job log
URL: https://github.com/apache/kyuubi/pull/5410
--
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
Re: [PR] [Improvement] spark showProgress with JobInfo #4186 [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 commented on PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#issuecomment-1759415480
> @davidyuan1223 thanks for your contribution, it's not necessary to put everything with screenshots, for example, it's clear to the reviewer to check your unit test code via GitHub. And would you mind filling in "Why are the changes needed?" in the PR description? Generally, for most of the PR, it's clear for the reviewer to check the code to understand "what you changed", but not "why are the changes needed".
>
> In addition, we provided some contributor guidance in #5357, please read and fix the code style issue.
ok, i will update the pr comment
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1357022249
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkConsoleProgressBar.scala:
##########
@@ -72,6 +73,19 @@ class SparkConsoleProgressBar(
}
}
+ /**
+ * Use stageId to find stage's jobId
+ * @param stageId stageId
+ * @return jobId,if not exists, return -1
+ */
+ private def findJobId(stageId: Int): Int = {
+ liveJobs.forEach((jobId, sparkJobInfo) => {
Review Comment:
Please change to scala code style, like:
```
import scala.collection.JavaConverters._
liveJobs.asScala.find(item => {
...
})
```
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1357026619
##########
externals/kyuubi-spark-sql-engine/src/test/scala/org/apache/spark/kyuubi/SQLOperationListenerSuite.scala:
##########
@@ -28,10 +28,33 @@ import org.apache.kyuubi.operation.HiveJDBCTestHelper
class SQLOperationListenerSuite extends WithSparkSQLEngine with HiveJDBCTestHelper {
- override def withKyuubiConf: Map[String, String] = Map.empty
+ override def withKyuubiConf: Map[String, String] = Map(
+ "kyuubi.session.engine.spark.showProgress" -> "true",
Review Comment:
use `KyuubiConf.XXX.key`
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1363224816
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SQLOperationListener.scala:
##########
@@ -20,6 +20,8 @@ package org.apache.spark.kyuubi
import java.util.Properties
import java.util.concurrent.ConcurrentHashMap
+import scala.jdk.CollectionConverters._
Review Comment:
change to:
```
import scala.collection.JavaConverters._
```
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1369494931
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkConsoleProgressBar.scala:
##########
@@ -81,7 +93,11 @@ class SparkConsoleProgressBar(
val width = TerminalWidth / stages.size
Review Comment:
i use the debug to test the width
1. this show the lastProgressbar show there only 1 stage, the bar show there have 3 stages
2. our terminalWidth default is 80, i think it's the best width to show progress info, the follow show when we make the default terminalWith is 180, it will show too long
there are 2 advice to improvement this
1. only display 1 stage at the same time
2. the not improvement logic is.
```
// length 9
val header = s"[Stage ${s.stageId}:"
// length 12~15
val tailer = s"(${s.numCompleteTasks} + ${s.numActiveTasks}) / $total]"
// if there have 3 stages, the width is 27. it will show 3~6 precent info
val w = width - header.length - tailer.length
```
we can add jobHeader.length in the expr `val w = width - header.length - tailer.length + jobHeader.lenght`, because we add job Info, so i think it's the best way
just like the photo , the bar show 3 stages at the same time, the lastProgressBar show only 1 stage at the same time
the photo is my debug and test,first and third line show only 1 stage display, second line show if 3 stages at the same time
what do you think? @wForget @cxzl25
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1361428894
##########
externals/kyuubi-spark-sql-engine/src/test/scala/org/apache/spark/kyuubi/SQLOperationListenerSuite.scala:
##########
@@ -22,16 +22,37 @@ import scala.collection.JavaConverters.asScalaBufferConverter
import org.apache.hive.service.rpc.thrift.{TExecuteStatementReq, TFetchOrientation, TFetchResultsReq, TOperationHandle}
import org.scalatest.time.SpanSugar._
+import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.config.KyuubiConf.OPERATION_SPARK_LISTENER_ENABLED
import org.apache.kyuubi.engine.spark.WithSparkSQLEngine
import org.apache.kyuubi.operation.HiveJDBCTestHelper
class SQLOperationListenerSuite extends WithSparkSQLEngine with HiveJDBCTestHelper {
- override def withKyuubiConf: Map[String, String] = Map.empty
+ override def withKyuubiConf: Map[String, String] = Map(
+ KyuubiConf.ENGINE_SPARK_SHOW_PROGRESS.key -> "true",
Review Comment:
https://github.com/apache/kyuubi/pull/5410#discussion_r1360162001
if we ignore the config position, just change test case order, the test is also success.
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1361442452
##########
externals/kyuubi-spark-sql-engine/src/test/scala/org/apache/spark/kyuubi/SQLOperationListenerSuite.scala:
##########
@@ -22,16 +22,37 @@ import scala.collection.JavaConverters.asScalaBufferConverter
import org.apache.hive.service.rpc.thrift.{TExecuteStatementReq, TFetchOrientation, TFetchResultsReq, TOperationHandle}
import org.scalatest.time.SpanSugar._
+import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.config.KyuubiConf.OPERATION_SPARK_LISTENER_ENABLED
import org.apache.kyuubi.engine.spark.WithSparkSQLEngine
import org.apache.kyuubi.operation.HiveJDBCTestHelper
class SQLOperationListenerSuite extends WithSparkSQLEngine with HiveJDBCTestHelper {
- override def withKyuubiConf: Map[String, String] = Map.empty
+ override def withKyuubiConf: Map[String, String] = Map(
+ KyuubiConf.ENGINE_SPARK_SHOW_PROGRESS.key -> "true",
Review Comment:
Yes, we can improve it in subsequent PRs, please take a look at https://github.com/apache/kyuubi/pull/5410#discussion_r1360160628 and use a faster sql instead of `java_method('java.lang. Thread', 'sleep', 10000l)`, like
```
select /*+ REPARTITION(3, a) */ a from values(1) t(a);
```
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#issuecomment-1765648511
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/5410?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#5410](https://app.codecov.io/gh/apache/kyuubi/pull/5410?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (9fa8e73) into [master](https://app.codecov.io/gh/apache/kyuubi/commit/e1d213ee9961a706cad47a263211ca8cb2c9057f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e1d213e) will **not change** coverage.
> Report is 15 commits behind head on master.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #5410 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 588 588
Lines 33427 33441 +14
Branches 4393 4392 -1
======================================
- Misses 33427 33441 +14
```
| [Files](https://app.codecov.io/gh/apache/kyuubi/pull/5410?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...in/scala/org/apache/spark/kyuubi/StageStatus.scala](https://app.codecov.io/gh/apache/kyuubi/pull/5410?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc3Bhcmsva3l1dWJpL1N0YWdlU3RhdHVzLnNjYWxh) | `0.00% <0.00%> (ø)` | |
| [...org/apache/spark/kyuubi/SQLOperationListener.scala](https://app.codecov.io/gh/apache/kyuubi/pull/5410?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc3Bhcmsva3l1dWJpL1NRTE9wZXJhdGlvbkxpc3RlbmVyLnNjYWxh) | `0.00% <0.00%> (ø)` | |
| [.../apache/spark/kyuubi/SparkConsoleProgressBar.scala](https://app.codecov.io/gh/apache/kyuubi/pull/5410?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc3Bhcmsva3l1dWJpL1NwYXJrQ29uc29sZVByb2dyZXNzQmFyLnNjYWxh) | `0.00% <0.00%> (ø)` | |
... and [2 files with indirect coverage changes](https://app.codecov.io/gh/apache/kyuubi/pull/5410/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1357015180
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SQLOperationListener.scala:
##########
@@ -79,37 +80,45 @@ class SQLOperationListener(
}
}
- override def onJobStart(jobStart: SparkListenerJobStart): Unit = activeJobs.synchronized {
- if (sameGroupId(jobStart.properties)) {
- val jobId = jobStart.jobId
- val stageSize = jobStart.stageInfos.size
- if (executionId.isEmpty) {
- executionId = Option(jobStart.properties.getProperty(SPARK_SQL_EXECUTION_ID_KEY))
- .map(_.toLong)
- consoleProgressBar
- operation match {
- case executeStatement: ExecuteStatement =>
- executeStatement.setCompiledStateIfNeeded()
- case _ =>
+ override def onJobStart(jobStart: SparkListenerJobStart): Unit = {
+ activeJobs.synchronized {
+ if (sameGroupId(jobStart.properties)) {
+ val jobId = jobStart.jobId
+ val stageIds = jobStart.stageInfos.map(_.stageId)
+ val stageSize = jobStart.stageInfos.size
+ if (executionId.isEmpty) {
+ executionId = Option(jobStart.properties.getProperty(SPARK_SQL_EXECUTION_ID_KEY))
+ .map(_.toLong)
+ consoleProgressBar
+ operation match {
+ case executeStatement: ExecuteStatement =>
+ executeStatement.setCompiledStateIfNeeded()
+ case _ =>
+ }
+ }
+ activeJobs.put(
+ jobId,
+ new SparkJobInfo(stageSize, stageIds)
+ )
+ withOperationLog {
+ info(s"Query [$operationId]: Job $jobId started with $stageSize stages," +
+ s" ${activeJobs.size()} active jobs running")
}
- }
- withOperationLog {
- activeJobs.add(jobId)
- info(s"Query [$operationId]: Job $jobId started with $stageSize stages," +
- s" ${activeJobs.size()} active jobs running")
}
}
}
override def onJobEnd(jobEnd: SparkListenerJobEnd): Unit = activeJobs.synchronized {
val jobId = jobEnd.jobId
- if (activeJobs.remove(jobId)) {
- val hint = jobEnd.jobResult match {
- case JobSucceeded => "succeeded"
- case _ => "failed" // TODO: Handle JobFailed(exception: Exception)
- }
- withOperationLog {
- info(s"Query [$operationId]: Job $jobId $hint, ${activeJobs.size()} active jobs running")
+ activeJobs.synchronized {
Review Comment:
duplicate `activeJobs.synchronized`
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "davidyuan1223 (via GitHub)" <gi...@apache.org>.
davidyuan1223 commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1357020232
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SQLOperationListener.scala:
##########
@@ -79,37 +80,45 @@ class SQLOperationListener(
}
}
- override def onJobStart(jobStart: SparkListenerJobStart): Unit = activeJobs.synchronized {
- if (sameGroupId(jobStart.properties)) {
- val jobId = jobStart.jobId
- val stageSize = jobStart.stageInfos.size
- if (executionId.isEmpty) {
- executionId = Option(jobStart.properties.getProperty(SPARK_SQL_EXECUTION_ID_KEY))
- .map(_.toLong)
- consoleProgressBar
- operation match {
- case executeStatement: ExecuteStatement =>
- executeStatement.setCompiledStateIfNeeded()
- case _ =>
+ override def onJobStart(jobStart: SparkListenerJobStart): Unit = {
+ activeJobs.synchronized {
Review Comment:
i think we can elminate synchronized
--
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
Re: [PR] [KYUUBI #4186] Spark showProgress with JobInfo [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #5410:
URL: https://github.com/apache/kyuubi/pull/5410#discussion_r1357024569
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/StageStatus.scala:
##########
@@ -27,3 +27,8 @@ class SparkStageInfo(val stageId: Int, val numTasks: Int) {
var numActiveTasks = new AtomicInteger(0)
var numCompleteTasks = new AtomicInteger(0)
}
+
+
+class SparkJobInfo(val numStages: Int, val stageIds: Seq[Int]) {
+ var numCompleteStages = new AtomicInteger(0)
+}
Review Comment:
end with newline
--
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