You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/09/03 03:35:37 UTC
[GitHub] [spark] ivoson opened a new pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
ivoson opened a new pull request #33905:
URL: https://github.com/apache/spark/pull/33905
### What changes were proposed in this pull request?
Now in QueryExecutionListener we have exposed API to get the query execution information:
def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit
def onFailure(funcName: String, qe: QueryExecution, exception: Exception): Unit
But we can not get a clear information that which query is this. In Spark SQL, I think that executionId is the identifier of a query execution. It makes sense to expose executionId to the QueryExecutionListener, so that people can easily find the exact query in UI or history server to track more information of the query execution.
### Why are the changes needed?
Easier to find the matching query in UI.
### Does this PR introduce _any_ user-facing change?
Developer Api change.
### How was this patch tested?
Existing UTs.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] closed pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #33905:
URL: https://github.com/apache/spark/pull/33905
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] commented on pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #33905:
URL: https://github.com/apache/spark/pull/33905#issuecomment-1006178610
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ivoson commented on pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
ivoson commented on pull request #33905:
URL: https://github.com/apache/spark/pull/33905#issuecomment-912410175
Building failure is due to the compatibility of the API change.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ivoson commented on a change in pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
ivoson commented on a change in pull request #33905:
URL: https://github.com/apache/spark/pull/33905#discussion_r703428815
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Observation.scala
##########
@@ -150,10 +150,12 @@ class Observation(name: String) {
private[sql] case class ObservationListener(observation: Observation)
extends QueryExecutionListener {
- override def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit =
+ override def onSuccess(
+ funcName: String, executionId: Long, qe: QueryExecution, durationNs: Long): Unit =
Review comment:
If we want to unify these two, I think the id bound to a QueryExecution should be the same with query execution id we can get from UI now. So the timing to initialize the id might also need to be the same.
Since a QueryExecution will be created for each dataset, we can not initialize the id for every QueryExecution, but just for the ones triggered by an action. I think the logic and timing here is just like what we did in SQLExecution.nextExecutionId.
In another word, we can just put the query execution Id to a QueryExecution when newExecutionId generated. This is what I have in mind now.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ivoson commented on pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
ivoson commented on pull request #33905:
URL: https://github.com/apache/spark/pull/33905#issuecomment-912370023
cc @cloud-fan could you please share thoughts about this change?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33905:
URL: https://github.com/apache/spark/pull/33905#discussion_r703241222
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Observation.scala
##########
@@ -150,10 +150,12 @@ class Observation(name: String) {
private[sql] case class ObservationListener(observation: Observation)
extends QueryExecutionListener {
- override def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit =
+ override def onSuccess(
+ funcName: String, executionId: Long, qe: QueryExecution, durationNs: Long): Unit =
Review comment:
can't we get the id by `qe.id`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ivoson commented on pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
ivoson commented on pull request #33905:
URL: https://github.com/apache/spark/pull/33905#issuecomment-927632880
cc @cloud-fan @HyukjinKwon Could you help review this?
Since a `QueryExecution` object can be bound to multi query executions, so we can not set a static executionId to a `QueryExecution` object.
I am thinking that maybe we can set executionId to a ThreadLocal variable, which can be fetched within callback function(onSuccess/onFailure).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ivoson commented on a change in pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
ivoson commented on a change in pull request #33905:
URL: https://github.com/apache/spark/pull/33905#discussion_r703428815
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Observation.scala
##########
@@ -150,10 +150,12 @@ class Observation(name: String) {
private[sql] case class ObservationListener(observation: Observation)
extends QueryExecutionListener {
- override def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit =
+ override def onSuccess(
+ funcName: String, executionId: Long, qe: QueryExecution, durationNs: Long): Unit =
Review comment:
If we want to unify these two, I think the id bound to a `QueryExecution` should be the same with query execution id we can get from UI now. So the timing to initialize the id might also need to be the same.
Since a `QueryExecution` will be created for each dataset, we can not initialize the id for every `QueryExecution`, but just for the ones triggered by an action. I think the logic and timing here is just like what we did in `SQLExecution.nextExecutionId`.
In another word, we can just put the query execution Id to a `QueryExecution` when newExecutionId generated. This is what I have in mind now.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ivoson commented on a change in pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
ivoson commented on a change in pull request #33905:
URL: https://github.com/apache/spark/pull/33905#discussion_r703328310
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Observation.scala
##########
@@ -150,10 +150,12 @@ class Observation(name: String) {
private[sql] case class ObservationListener(observation: Observation)
extends QueryExecutionListener {
- override def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit =
+ override def onSuccess(
+ funcName: String, executionId: Long, qe: QueryExecution, durationNs: Long): Unit =
Review comment:
If use QueryExecution.id, the queryId in SQL page of UI may not be sequential if I got the right understanding. If it is the case, would that be a 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33905:
URL: https://github.com/apache/spark/pull/33905#discussion_r703342901
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Observation.scala
##########
@@ -150,10 +150,12 @@ class Observation(name: String) {
private[sql] case class ObservationListener(observation: Observation)
extends QueryExecutionListener {
- override def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit =
+ override def onSuccess(
+ funcName: String, executionId: Long, qe: QueryExecution, durationNs: Long): Unit =
Review comment:
I see, can we make `QueryExecution.id` sequential?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33905:
URL: https://github.com/apache/spark/pull/33905#discussion_r703242758
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Observation.scala
##########
@@ -150,10 +150,12 @@ class Observation(name: String) {
private[sql] case class ObservationListener(observation: Observation)
extends QueryExecutionListener {
- override def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit =
+ override def onSuccess(
+ funcName: String, executionId: Long, qe: QueryExecution, durationNs: Long): Unit =
Review comment:
oh they are not the same thing. We should probably unify these two
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ivoson commented on a change in pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
ivoson commented on a change in pull request #33905:
URL: https://github.com/apache/spark/pull/33905#discussion_r703458145
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Observation.scala
##########
@@ -150,10 +150,12 @@ class Observation(name: String) {
private[sql] case class ObservationListener(observation: Observation)
extends QueryExecutionListener {
- override def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit =
+ override def onSuccess(
+ funcName: String, executionId: Long, qe: QueryExecution, durationNs: Long): Unit =
Review comment:
But this also has some problems, since a `QueryExecution`/`Dataset` can be reused for query execution multi times, so we may get same `QueryExecution` with different execution ids. Seems not that straightforward to unify these 2 ids...
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ivoson commented on a change in pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
ivoson commented on a change in pull request #33905:
URL: https://github.com/apache/spark/pull/33905#discussion_r703286383
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Observation.scala
##########
@@ -150,10 +150,12 @@ class Observation(name: String) {
private[sql] case class ObservationListener(observation: Observation)
extends QueryExecutionListener {
- override def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit =
+ override def onSuccess(
+ funcName: String, executionId: Long, qe: QueryExecution, durationNs: Long): Unit =
Review comment:
Checked the original motivation of the queryExecution.id in this PR https://github.com/apache/spark/pull/29372. Maybe take query execution id also works for it.
Then maybe we can lazily set a copy of query execution id to QeuryExecution object when [SQLExecution.withNewExecutionId](https://github.com/apache/spark/blob/v3.2.0-rc2/sql/core/src/main/scala/org/apache/spark/sql/execution/SQLExecution.scala#L62) is invoked and query execution id is generated.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ivoson edited a comment on pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
ivoson edited a comment on pull request #33905:
URL: https://github.com/apache/spark/pull/33905#issuecomment-912410175
Building failure is due to the compatibility check because of the API change.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ivoson commented on a change in pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
ivoson commented on a change in pull request #33905:
URL: https://github.com/apache/spark/pull/33905#discussion_r703458145
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Observation.scala
##########
@@ -150,10 +150,12 @@ class Observation(name: String) {
private[sql] case class ObservationListener(observation: Observation)
extends QueryExecutionListener {
- override def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit =
+ override def onSuccess(
+ funcName: String, executionId: Long, qe: QueryExecution, durationNs: Long): Unit =
Review comment:
But this also has some problems, since a `QueryExecution`/`Dataset` can be reused for query execution multi times, so we may get same `QueryExecution` with different execution ids.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33905:
URL: https://github.com/apache/spark/pull/33905#issuecomment-912229215
Can one of the admins verify this patch?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #33905:
URL: https://github.com/apache/spark/pull/33905#discussion_r703318930
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Observation.scala
##########
@@ -150,10 +150,12 @@ class Observation(name: String) {
private[sql] case class ObservationListener(observation: Observation)
extends QueryExecutionListener {
- override def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit =
+ override def onSuccess(
+ funcName: String, executionId: Long, qe: QueryExecution, durationNs: Long): Unit =
Review comment:
one idea is to remove `SQLExecution.nextExecutionId` and always use `QueryExecution.id`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ivoson commented on a change in pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
ivoson commented on a change in pull request #33905:
URL: https://github.com/apache/spark/pull/33905#discussion_r703271980
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Observation.scala
##########
@@ -150,10 +150,12 @@ class Observation(name: String) {
private[sql] case class ObservationListener(observation: Observation)
extends QueryExecutionListener {
- override def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit =
+ override def onSuccess(
+ funcName: String, executionId: Long, qe: QueryExecution, durationNs: Long): Unit =
Review comment:
Yes, they are not the same. Definitely, it will be great if we can unify 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ivoson commented on a change in pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
ivoson commented on a change in pull request #33905:
URL: https://github.com/apache/spark/pull/33905#discussion_r703271980
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Observation.scala
##########
@@ -150,10 +150,12 @@ class Observation(name: String) {
private[sql] case class ObservationListener(observation: Observation)
extends QueryExecutionListener {
- override def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit =
+ override def onSuccess(
+ funcName: String, executionId: Long, qe: QueryExecution, durationNs: Long): Unit =
Review comment:
Yes, they are not the same. Definitely, it will great if we can unify 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #33905:
URL: https://github.com/apache/spark/pull/33905#discussion_r703245225
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/util/QueryExecutionListener.scala
##########
@@ -41,19 +41,21 @@ trait QueryExecutionListener {
* A callback function that will be called when a query executed successfully.
*
* @param funcName name of the action that triggered this query.
+ * @param executionId identifier of a query execution.
* @param qe the QueryExecution object that carries detail information like logical plan,
* physical plan, etc.
* @param durationNs the execution time for this query in nanoseconds.
*
* @note This can be invoked by multiple different threads.
*/
@DeveloperApi
- def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit
+ def onSuccess(funcName: String, executionId: Long, qe: QueryExecution, durationNs: Long): Unit
Review comment:
we can't just add like this .. it breaks binary compatibility
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ivoson commented on a change in pull request #33905: [SPARK-36658][SQL] Expose execution id to QueryExecutionListener
Posted by GitBox <gi...@apache.org>.
ivoson commented on a change in pull request #33905:
URL: https://github.com/apache/spark/pull/33905#discussion_r703273447
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/util/QueryExecutionListener.scala
##########
@@ -41,19 +41,21 @@ trait QueryExecutionListener {
* A callback function that will be called when a query executed successfully.
*
* @param funcName name of the action that triggered this query.
+ * @param executionId identifier of a query execution.
* @param qe the QueryExecution object that carries detail information like logical plan,
* physical plan, etc.
* @param durationNs the execution time for this query in nanoseconds.
*
* @note This can be invoked by multiple different threads.
*/
@DeveloperApi
- def onSuccess(funcName: String, qe: QueryExecution, durationNs: Long): Unit
+ def onSuccess(funcName: String, executionId: Long, qe: QueryExecution, durationNs: Long): Unit
Review comment:
Yeah...That is also my concerns about 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org