Skip to content
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.

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

[SPARK-27142][SQL] Provide REST API for SQL information #24076

Closed
wants to merge 15 commits into from

Conversation

ajithme
Copy link
Contributor

@ajithme ajithme commented Mar 13, 2019

What changes were proposed in this pull request?

Currently for Monitoring Spark application SQL information is not available from REST but only via UI. REST provides only applications,jobs,stages,environment. This Jira is targeted to provide a REST API so that SQL level information can be found
A single SQL query can result into multiple jobs. So for end user who is using STS or spark-sql, the intended highest level of probe is the SQL which he has executed. This information can be seen from SQL tab. Attaching a sample.
image
But same information he cannot access using the REST API exposed by spark and he always have to rely on jobs API which may be difficult. So i intend to expose the information seen in SQL tab in UI via REST API

Mainly:

Id : Long - execution id of the sql
status : String - possible values COMPLETED/RUNNING/FAILED
description : String - executed SQL string
planDescription : String - Plan representation
metrics : Seq[Metrics] - Metrics contain metricName: String, metricValue: String
submissionTime : String - formatted Date time of SQL submission
duration : Long - total run time in milliseconds
runningJobIds : Seq[Int] - sequence of running job ids
failedJobIds : Seq[Int] - sequence of failed job ids
successJobIds : Seq[Int] - sequence of success job ids

  • To fetch sql executions: /sql?details=boolean&offset=integer&length=integer
  • To fetch single execution: /sql/{executionID}?details=boolean
parameter type remarks
details boolean Optional. Set true to get plan description and metrics information, defaults to false
offset integer Optional. offset to fetch the executions, defaults to 0
length integer Optional. total number of executions to be fetched, defaults to 20

Why are the changes needed?

To support users query SQL information via REST API

Does this PR introduce any user-facing change?

Yes. It provides a new monitoring URL for SQL

How was this patch tested?

Tested manually

image

image

@ajithme
Copy link
Contributor Author

ajithme commented Mar 13, 2019

@gengliangwang
Copy link
Member

@ajithme Thanks for the work.
To make it consistent with other API, I think we need to have at least two APIs

@Path("sql")
@Path("sql/{sqlExecutionId: \\d+}")

@ajithme
Copy link
Contributor Author

ajithme commented Mar 13, 2019

@ajithme Thanks for the work.
To make it consistent with other API, I think we need to have at least two APIs

@Path("sql")
@Path("sql/{sqlExecutionId: \\d+}")

@gengliangwang So the full api looks like

http://localhost:4040/api/v1/applications/{applicationId}/sql
http://localhost:4040/api/v1/applications/{applicationId}/sql/{executionId}

is this the expectation.?

@gengliangwang
Copy link
Member

http://localhost:4040/api/v1/applications/{applicationId}/sql
http://localhost:4040/api/v1/applications/{applicationId}/sql/{executionId}

Yes, it is consistent with

http://localhost:4040/api/v1/applications/{applicationId}/jobs/
http://localhost:4040/api/v1/applications/{applicationId}/jobs/{jobId}

@ajithme
Copy link
Contributor Author

ajithme commented Mar 13, 2019

Ok i have updated the PR accordingly. Please review

@dongjoon-hyun
Copy link
Member

ok to test

@SparkQA
Copy link

SparkQA commented Mar 13, 2019

Test build #103450 has started for PR 24076 at commit 8827301.

@ajithme
Copy link
Contributor Author

ajithme commented Mar 14, 2019

gentle ping @gengliangwang @dongjoon-hyun @srowen

@srowen
Copy link
Member

srowen commented Mar 14, 2019

As I say I'm not sure this is something we should expose this way. See prior comments on JIRA

@ajithme
Copy link
Contributor Author

ajithme commented Mar 17, 2019

@srowen i have updated the use case in the JIRA already for prior comments. Please have a look

@ajithme
Copy link
Contributor Author

ajithme commented Mar 19, 2019

gentle ping @cloud-fan @srowen @dongjoon-hyun @gengliangwang Any further suggestions of this feature.?

@gengliangwang
Copy link
Member

gengliangwang commented Mar 20, 2019

@ajithme I am +1 for this.
Some comments are not addressed correctly(e.g. #24076 (comment)), so I create a PR(ajithme#1) in your repo. Please review it.

@ajithme
Copy link
Contributor Author

ajithme commented Mar 26, 2019

@ajithme I am +1 for this.
Some comments are not addressed correctly(e.g. #24076 (comment)), so I create a PR(ajithme#1) in your repo. Please review it.

Thanks @gengliangwang I got confused between core/src/main/scala/org/apache/spark/status/api/v1/api.scala and sql/core/src/main/scala/org/apache/spark/status/api/v1/api.scala as file names are same :) . i will merge your comments shortly here.

@ajithme
Copy link
Contributor Author

ajithme commented Mar 26, 2019

Updated with latest comments fixed. Please review

@SparkQA
Copy link

SparkQA commented Mar 26, 2019

Test build #103950 has finished for PR 24076 at commit b1a8deb.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gengliangwang
Copy link
Member

retest this please.

@SparkQA
Copy link

SparkQA commented Mar 26, 2019

Test build #103965 has finished for PR 24076 at commit b1a8deb.

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

@gengliangwang
Copy link
Member

@vanzin

@ajithme
Copy link
Contributor Author

ajithme commented May 6, 2019

@vanzin @srowen @dongjoon-hyun gentle ping. Can we re-look into this.? its been open for quite sometime

Copy link
Contributor

@vanzin vanzin left a comment

Choose a reason for hiding this comment

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

This seems to be missing the plan description and metrics. Any specific reason not to expose those?

@ajithme
Copy link
Contributor Author

ajithme commented Aug 9, 2019

This seems to be missing the plan description and metrics. Any specific reason not to expose those?

@vanzin Wanted this API to be light weight for monitoring. Returning plan and metrics can significantly increase the payload. Please share your thoughts

@vanzin
Copy link
Contributor

vanzin commented Aug 15, 2019

Returning plan and metrics can significantly increase the payload.

Yes, but it's also useful information and there would be no way to retrieve it from the API...

In our APIs we generally have a query parameter that tells you what level of detail you have in the response, so you can have a small summary or a full view of an object, for example. That sounds like something we could add here.

I even did something similar in the StagesResource.oneAttemptData method with the details parameter. You could add that parameter here with a default value of false, for example, so only those who really want the extra data would get it.

@ajithme
Copy link
Contributor Author

ajithme commented Aug 15, 2019

Returning plan and metrics can significantly increase the payload.

Yes, but it's also useful information and there would be no way to retrieve it from the API...

In our APIs we generally have a query parameter that tells you what level of detail you have in the response, so you can have a small summary or a full view of an object, for example. That sounds like something we could add here.

I even did something similar in the StagesResource.oneAttemptData method with the details parameter. You could add that parameter here with a default value of false, for example, so only those who really want the extra data would get it.

Thanks @vanzin for inputs. I agree with your opinion and will update the PR

@ajithme
Copy link
Contributor Author

ajithme commented Jan 8, 2020

After handling review comments

image

@ajithme
Copy link
Contributor Author

ajithme commented Jan 8, 2020

@vanzin Thank you for review and i apologise for indent issues. Now i have run ./dev/scalafmt and updated the PR with other review comments. Please review

@ajithme ajithme requested a review from vanzin January 10, 2020 06:16
Copy link
Contributor

@vanzin vanzin left a comment

Choose a reason for hiding this comment

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

Few minor things.

@ajithme
Copy link
Contributor Author

ajithme commented Jan 10, 2020

@vanzin please review, updated as per the comments

@vanzin
Copy link
Contributor

vanzin commented Jan 10, 2020

Weird tests aren't running.

@vanzin
Copy link
Contributor

vanzin commented Jan 10, 2020

ok to test

@SparkQA
Copy link

SparkQA commented Jan 11, 2020

Test build #116509 has finished for PR 24076 at commit 4560df9.

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

@ajithme
Copy link
Contributor Author

ajithme commented Jan 14, 2020

@vanzin gentle ping

@vanzin
Copy link
Contributor

vanzin commented Jan 14, 2020

Merging to master.

@gengliangwang
Copy link
Member

@ajithme I think we have to revert this one in branch-3.0. See my comments in #28588

cloud-fan pushed a commit that referenced this pull request May 20, 2020
### What changes were proposed in this pull request?

Revert #28208 and #24076 in branch 3.0
### Why are the changes needed?

Unfortunately, the PR  #28208 is merged after Spark 3.0 RC 2 cut. Although the improvement is great, we can't break the policy to add new improvement commits into branch 3.0 now.

Also, if we are going to adopt the improvement in a future release, we should not release 3.0 with #24076, since the API result will be changed.

After discuss with cloud-fan and gatorsmile offline, we think the best choice is to revert both commits and follow community release policy.

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

Yes, let's hold the SQL rest API until next release.

### How was this patch tested?

Jenkins unit tests.

Closes #28588 from gengliangwang/revertSQLRestAPI.

Authored-by: Gengliang Wang <gengliang.wang@databricks.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants