Skip to content

[SPARK-28556][SQL] QueryExecutionListener should also notify Error #25292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
zsxwing wants to merge 3 commits into from
Closed

[SPARK-28556][SQL] QueryExecutionListener should also notify Error #25292

zsxwing wants to merge 3 commits into from

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented Jul 29, 2019

What changes were proposed in this pull request?

Right now Error is not sent to QueryExecutionListener.onFailure. If there is any Error (such as AssertionError) when running a query, QueryExecutionListener.onFailure cannot be triggered.

This PR changes onFailure to accept a Throwable instead.

How was this patch tested?

Jenkins

@zsxwing zsxwing added the SQL label Jul 29, 2019
@zsxwing zsxwing requested a review from cloud-fan July 29, 2019 20:34
zsxwing
zsxwing commented Jul 29, 2019
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/util/QueryExecutionListener.scala
*/
@DeveloperApi
def onFailure(funcName: String, qe: QueryExecution, exception: Exception): Unit
def onFailure(funcName: String, qe: QueryExecution, error: Throwable): Unit
Copy link
Member Author

@zsxwing zsxwing Jul 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need a release note.

Copy link
Member Author

@zsxwing zsxwing Jul 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another issue in this API: It passes a private class QueryExecution to the user in a public API. Didn't fix it as it will require a re-design of this API.

Copy link
Member

@HyukjinKwon HyukjinKwon Jul 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we should sort those problems out. There's another occurrence at Dataset.queryExecution as an API.

Copy link
Contributor

@marmbrus marmbrus Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned about this change. This broke one of my project that uses this API. Instead of changing the signature here, did we consider just wrapping the exception?

Copy link
Contributor

@marmbrus marmbrus Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RE: QueryExecution I think that has become a developer API at this point. I think a lot of developers use it to debug things.

Copy link
Member Author

@zsxwing zsxwing Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrapping the exception?

This is a good suggestion. This can avoid api changes, and we can also fix this bug that errors are not sent to the listener in 2.4.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-28556][SQL]QueryExecutionListener should also notify Error [SPARK-28556][SQL] QueryExecutionListener should also notify Error Jul 29, 2019
@SparkQA
Copy link

SparkQA commented Jul 29, 2019

Test build #108345 has finished for PR 25292 at commit 82e1800.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 29, 2019

Test build #108348 has finished for PR 25292 at commit 8fb2c5f.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 29, 2019

Test build #108349 has finished for PR 25292 at commit bdc2c26.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

dongjoon-hyun
dongjoon-hyun approved these changes Jul 30, 2019
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM.

HyukjinKwon
HyukjinKwon approved these changes Jul 30, 2019
View reviewed changes
Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Jul 30, 2019

Merged to master.

@zsxwing zsxwing deleted the fix-QueryExecutionListener branch July 30, 2019 05:43
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 29, 2020 •
edited
Loading

Hi, @gatorsmile , @zsxwing , @HyukjinKwon .
If we need to mention this at 2.4.5 release-note, please let me know.
2.4.5 is the last version before 3.0.0.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Mar 11, 2020

For the following, cc @rxin since he is a release manager for 3.0.0. (Also, cc @gatorsmile ).

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Mar 12, 2020

Hi, @marmbrus .
This was discussed legitimately at that time here and on JIRA.
If you want to revert this, please make a new legitimate PR. We will review that carefully according to the community policy (AS-IS).

@marmbrus
Copy link
Contributor

marmbrus commented Mar 12, 2020

I don't see any discussion of alternatives to breaking the API here or on JIRA. I'm just asking that we discuss. I don't see anything "illegitimate" about that... We can open a JIRA after others have a chance to chime in.

@zsxwing
Copy link
Member Author

zsxwing commented Mar 13, 2020

I didn't think about wrapping the exception. Totally agree that this is actually a better solution. It can avoid breaking APIs , and in addition, the same fix can be applied to maintenance branches (such as branch-2.4). I will create a new JIRA ticket for this alternative solution.

@zsxwing
Copy link
Member Author

zsxwing commented Mar 13, 2020

Created https://issues.apache.org/jira/browse/SPARK-31144

@zsxwing zsxwing mentioned this pull request Mar 13, 2020
Closed
dongjoon-hyun pushed a commit that referenced this pull request Mar 13, 2020
@zsxwing @dongjoon-hyun
[SPARK-31144][SQL] Wrap Error with QueryExecutionException to notify …
339e4dd 
…QueryExecutionListener

### What changes were proposed in this pull request?

This PR manually reverts changes in #25292 and then wraps java.lang.Error with `QueryExecutionException` to notify `QueryExecutionListener` to send it to `QueryExecutionListener.onFailure` which only accepts `Exception`.

The bug fix PR for 2.4 is #27904. It needs a separate PR because the touched codes were changed a lot.

### Why are the changes needed?

Avoid API changes and fix a bug.

### Does this PR introduce any user-facing change?

Yes. Reverting an API change happening in 3.0. QueryExecutionListener APIs will be the same as 2.4.

### How was this patch tested?

The new added test.

Closes #27907 from zsxwing/SPARK-31144.

Authored-by: Shixiong Zhu <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 1ddf44d)
Signed-off-by: Dongjoon Hyun <[email protected]>
dongjoon-hyun pushed a commit that referenced this pull request Mar 13, 2020
@zsxwing @dongjoon-hyun
[SPARK-31144][SQL] Wrap Error with QueryExecutionException to notify …
1ddf44d 
…QueryExecutionListener

### What changes were proposed in this pull request?

This PR manually reverts changes in #25292 and then wraps java.lang.Error with `QueryExecutionException` to notify `QueryExecutionListener` to send it to `QueryExecutionListener.onFailure` which only accepts `Exception`.

The bug fix PR for 2.4 is #27904. It needs a separate PR because the touched codes were changed a lot.

### Why are the changes needed?

Avoid API changes and fix a bug.

### Does this PR introduce any user-facing change?

Yes. Reverting an API change happening in 3.0. QueryExecutionListener APIs will be the same as 2.4.

### How was this patch tested?

The new added test.

Closes #27907 from zsxwing/SPARK-31144.

Authored-by: Shixiong Zhu <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
sjincho pushed a commit to sjincho/spark that referenced this pull request Apr 15, 2020
@zsxwing
[SPARK-31144][SQL] Wrap Error with QueryExecutionException to notify …
f937e26 
…QueryExecutionListener

### What changes were proposed in this pull request?

This PR manually reverts changes in apache#25292 and then wraps java.lang.Error with `QueryExecutionException` to notify `QueryExecutionListener` to send it to `QueryExecutionListener.onFailure` which only accepts `Exception`.

The bug fix PR for 2.4 is apache#27904. It needs a separate PR because the touched codes were changed a lot.

### Why are the changes needed?

Avoid API changes and fix a bug.

### Does this PR introduce any user-facing change?

Yes. Reverting an API change happening in 3.0. QueryExecutionListener APIs will be the same as 2.4.

### How was this patch tested?

The new added test.

Closes apache#27907 from zsxwing/SPARK-31144.

Authored-by: Shixiong Zhu <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

@cloud-fan cloud-fan Awaiting requested review from cloud-fan

+1 more reviewer

@marmbrus marmbrus marmbrus left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zsxwing @SparkQA @HyukjinKwon @dongjoon-hyun @marmbrus