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/05/10 08:11:14 UTC
[GitHub] [incubator-kyuubi] cxzl25 opened a new pull request, #2617: [KYUUBI #2605] Make SQLOperationListener configurable
cxzl25 opened a new pull request, #2617:
URL: https://github.com/apache/incubator-kyuubi/pull/2617
### _Why are the changes needed?_
close #2605
`kyuubi.engine.spark.register.operation.listener`
> When set to true, Spark engine registers a SQLOperationListener before executing the statement, logs a few summary statistics when each stage completes.
### _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] codecov-commenter commented on pull request #2617: [KYUUBI #2605] Make SQLOperationListener configurable
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2617:
URL: https://github.com/apache/incubator-kyuubi/pull/2617#issuecomment-1122284037
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2617?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 [#2617](https://codecov.io/gh/apache/incubator-kyuubi/pull/2617?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a7b29de) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/1fee068c99802b68f6fb1f7572050b054a9fef01?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1fee068) will **increase** coverage by `0.00%`.
> The diff coverage is `85.71%`.
```diff
@@ Coverage Diff @@
## master #2617 +/- ##
=========================================
Coverage 63.62% 63.62%
Complexity 79 79
=========================================
Files 380 380
Lines 18516 18525 +9
Branches 2507 2508 +1
=========================================
+ Hits 11780 11787 +7
- Misses 5622 5623 +1
- Partials 1114 1115 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/2617?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...uubi/engine/spark/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2617/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `81.81% <77.77%> (-1.52%)` | :arrow_down: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2617/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.62% <100.00%> (-0.10%)` | :arrow_down: |
| [...ache/kyuubi/engine/spark/SparkProcessBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2617/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvc3BhcmsvU3BhcmtQcm9jZXNzQnVpbGRlci5zY2FsYQ==) | `83.60% <0.00%> (-1.64%)` | :arrow_down: |
| [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2617/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvY0J1aWxkZXIuc2NhbGE=) | `84.90% <0.00%> (+0.62%)` | :arrow_up: |
| [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2617/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==) | `85.24% <0.00%> (+1.63%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2617?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/2617?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 [1fee068...a7b29de](https://codecov.io/gh/apache/incubator-kyuubi/pull/2617?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] yaooqinn commented on a diff in pull request #2617: [KYUUBI #2605] Make SQLOperationListener configurable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2617:
URL: https://github.com/apache/incubator-kyuubi/pull/2617#discussion_r869418721
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala:
##########
@@ -42,13 +41,19 @@ class ExecuteStatement(
override val statement: String,
override val shouldRunAsync: Boolean,
queryTimeout: Long,
- incrementalCollect: Boolean)
+ incrementalCollect: Boolean,
+ operationSparkListenerEnabled: Boolean)
Review Comment:
refer to progressEnable？
--
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] ulysses-you commented on pull request #2617: [KYUUBI #2605] Make SQLOperationListener configurable
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #2617:
URL: https://github.com/apache/incubator-kyuubi/pull/2617#issuecomment-1122197857
@cxzl25 please resolve the conflicts, thank you
--
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 #2617: [KYUUBI #2605] Make SQLOperationListener configurable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2617:
URL: https://github.com/apache/incubator-kyuubi/pull/2617#discussion_r869083381
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1499,4 +1499,12 @@ object KyuubiConf {
.version("1.6.0")
.regexConf
.createOptional
+
+ val ENGINE_SPARK_REGISTER_OPERATION_LISTENER: ConfigEntry[Boolean] =
+ buildConf("kyuubi.engine.spark.register.operation.listener")
Review Comment:
kyuubi.operation.spark.listener.enabled
--
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] cxzl25 commented on a diff in pull request #2617: [KYUUBI #2605] Make SQLOperationListener configurable
Posted by GitBox <gi...@apache.org>.
cxzl25 commented on code in PR #2617:
URL: https://github.com/apache/incubator-kyuubi/pull/2617#discussion_r870910223
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala:
##########
@@ -48,7 +48,14 @@ class ExecuteStatement(
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
override def getOperationLog: Option[OperationLog] = Option(operationLog)
- private val operationListener: SQLOperationListener = new SQLOperationListener(this, spark)
+ private val conf: KyuubiConf = getSession.sessionManager.getConf
+
+ private val operationListener: Option[SQLOperationListener] =
Review Comment:
It should be solved, I add set true/false in UT to test.
Thanks for your review.
--
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 #2617: [KYUUBI #2605] Make SQLOperationListener configurable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #2617:
URL: https://github.com/apache/incubator-kyuubi/pull/2617#issuecomment-1122218198
Can we also add an unit test for 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
[GitHub] [incubator-kyuubi] yaooqinn closed pull request #2617: [KYUUBI #2605] Make SQLOperationListener configurable
Posted by GitBox <gi...@apache.org>.
yaooqinn closed pull request #2617: [KYUUBI #2605] Make SQLOperationListener configurable
URL: https://github.com/apache/incubator-kyuubi/pull/2617
--
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 #2617: [KYUUBI #2605] Make SQLOperationListener configurable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2617:
URL: https://github.com/apache/incubator-kyuubi/pull/2617#discussion_r869080919
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala:
##########
@@ -48,7 +48,14 @@ class ExecuteStatement(
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
override def getOperationLog: Option[OperationLog] = Option(operationLog)
- private val operationListener: SQLOperationListener = new SQLOperationListener(this, spark)
+ private val conf: KyuubiConf = getSession.sessionManager.getConf
+
+ private val operationListener: Option[SQLOperationListener] =
Review Comment:
we can make it settable via sql command
--
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 #2617: [KYUUBI #2605] Make SQLOperationListener configurable
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #2617:
URL: https://github.com/apache/incubator-kyuubi/pull/2617#issuecomment-1124479864
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] ulysses-you commented on a diff in pull request #2617: [KYUUBI #2605] Make SQLOperationListener configurable
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #2617:
URL: https://github.com/apache/incubator-kyuubi/pull/2617#discussion_r870865645
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala:
##########
@@ -48,7 +48,14 @@ class ExecuteStatement(
private val operationLog: OperationLog = OperationLog.createOperationLog(session, getHandle)
override def getOperationLog: Option[OperationLog] = Option(operationLog)
- private val operationListener: SQLOperationListener = new SQLOperationListener(this, spark)
+ private val conf: KyuubiConf = getSession.sessionManager.getConf
+
+ private val operationListener: Option[SQLOperationListener] =
Review Comment:
@cxzl25 seems this comment has not been addressed ? can you take a look ?
--
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