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-34019][WEBUI] Keep same quantiles of task summary between Web UI and restful API #31048

Closed
wants to merge 3 commits into from

Conversation

AngersZhuuuu
Copy link
Contributor

What changes were proposed in this pull request?

We should keep same quantiles about task summary about Web UI and Restful API

The direct result of resuful API is not same with Web UI is confusing. We'd better to keep same.

Why are the changes needed?

Keep same result between restful API and web ui

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existed UI

@AngersZhuuuu
Copy link
Contributor Author

ping @gengliangwang @sarutak
found this when we discuss about #31001 (comment)
We should keep all consistent. also. cc @ron8hu

@github-actions github-actions bot added the CORE label Jan 6, 2021
@SparkQA
Copy link

SparkQA commented Jan 6, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38284/

@SparkQA
Copy link

SparkQA commented Jan 6, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38284/

@AngersZhuuuu AngersZhuuuu changed the title [SPARK-34019][WEBUI] Keep same quantiles about task summary about Web UI and restful API [SPARK-34019][WEBUI] Keep same quantiles of task summary between Web UI and restful API Jan 6, 2021
@SparkQA
Copy link

SparkQA commented Jan 6, 2021

Test build #133696 has finished for PR 31048 at commit c0e05b7.

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

@SparkQA
Copy link

SparkQA commented Jan 6, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38294/

@SparkQA
Copy link

SparkQA commented Jan 6, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38294/

@SparkQA
Copy link

SparkQA commented Jan 6, 2021

Test build #133706 has finished for PR 31048 at commit 6ae763e.

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

@AngersZhuuuu
Copy link
Contributor Author

ping @squito @dongjoon-hyun @srowen @tgravescs since you have commit to this part.

@sarutak
Copy link
Member

sarutak commented Jan 8, 2021

The UI for a stage shows 0,0, 0.25, 0.50, 0.75 and 1.0 percentiles so it seems to be consistent.

"data": taskSummaryMetricsTable,
"columns": [
{data : 'metric'},
// Min
{
data: function (row, type) {
return displayRowsForSummaryMetricsTable(row, type, 0);
}
},
// 25th percentile
{
data: function (row, type) {
return displayRowsForSummaryMetricsTable(row, type, 1);
}
},
// Median
{
data: function (row, type) {
return displayRowsForSummaryMetricsTable(row, type, 2);
}
},
// 75th percentile
{
data: function (row, type) {
return displayRowsForSummaryMetricsTable(row, type, 3);
}
},
// Max
{
data: function (row, type) {
return displayRowsForSummaryMetricsTable(row, type, 4);
}
}

@AngersZhuuuu
Copy link
Contributor Author

The UI for a stage shows 0,0, 0.25, 0.50, 0.75 and 1.0 percentiles so it seems to be consistent.

"data": taskSummaryMetricsTable,
"columns": [
{data : 'metric'},
// Min
{
data: function (row, type) {
return displayRowsForSummaryMetricsTable(row, type, 0);
}
},
// 25th percentile
{
data: function (row, type) {
return displayRowsForSummaryMetricsTable(row, type, 1);
}
},
// Median
{
data: function (row, type) {
return displayRowsForSummaryMetricsTable(row, type, 2);
}
},
// 75th percentile
{
data: function (row, type) {
return displayRowsForSummaryMetricsTable(row, type, 3);
}
},
// Max
{
data: function (row, type) {
return displayRowsForSummaryMetricsTable(row, type, 4);
}
}

But restful is 0.05,0.25,0.5,0.75,0.95

@sarutak
Copy link
Member

sarutak commented Jan 8, 2021

But restful is 0.05,0.25,0.5,0.75,0.95

Yes, I understand.
I mean the change seems reasonable to align to the UI.

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Jan 8, 2021

Yes, I understand.
I mean the change seems reasonable to align to the UI.

Ur, you mean change UI to be consistent with RestFul API. Is there any discuss about why use 0.05,0.25,0.5,0.75,0.95?

@sarutak
Copy link
Member

sarutak commented Jan 8, 2021

Ur, you mean change UI to be consistent with RestFul API. Is there any discuss about why use 0.05,0.25,0.5,0.75,0.95?

No, I don't think it's necessary to change the UI. This change seems reasonable to conform to the UI.
I saw the past PR but I don't see the discussion about the quantile.

@AngersZhuuuu
Copy link
Contributor Author

Ur, you mean change UI to be consistent with RestFul API. Is there any discuss about why use 0.05,0.25,0.5,0.75,0.95?

No, I don't think it's necessary to change the UI. This change seems reasonable to conform to the UI.
I saw the past PR but I don't see the discussion about the quantile.

Yea

@AngersZhuuuu
Copy link
Contributor Author

retest this please

@SparkQA
Copy link

SparkQA commented Jan 8, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38419/

@SparkQA
Copy link

SparkQA commented Jan 8, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38419/

@SparkQA
Copy link

SparkQA commented Jan 8, 2021

Test build #133830 has finished for PR 31048 at commit 6ae763e.

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

@srowen
Copy link
Member

srowen commented Jan 8, 2021

What does the UI look like before and after? what is in the REST API not in the UI?

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Jan 8, 2021

What does the UI look like before and after? what is in the REST API not in the UI?

The problem is :

  1. In spark web ui task summary the quantiles is 0.0.0.25,0.5,0.75,1.0
  2. In restful API /applications/[app-id]/stages/[stage-id]/[stage-attempt-id]/taskSummary the quantiles is 0.05,0.25,0.5,0.75,0.95

The result is not same, we'd better keep it consistent

For one APP running in our env, stage=4 & stageAttemptId=0
We can see the scheduler delay. the value is not same.
Screen Shot 2021-01-08 at 10 03 21 PM
Screen Shot 2021-01-08 at 10 03 46 PM

@tgravescs
Copy link
Contributor

those are just default values so the user can change what they request. Normally I would agree that keeping the same makes sense. I tried to go back to see if there was a reason it was made this way but didn't see any comments on it.

One question is whether this is a breaking change because users that do this will now get a different result. thoughts from others? Ideally it doesn't matter as they would check what the quantiles are but if they aren't it would be different results.

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Jan 8, 2021

those are just default values so the user can change what they request. Normally I would agree that keeping the same makes sense. I tried to go back to see if there was a reason it was made this way but didn't see any comments on it.

One question is whether this is a breaking change because users that do this will now get a different result. thoughts from others? Ideally it doesn't matter as they would check what the quantiles are but if they aren't it would be different results.

Yea, and default value is not show in monitoring page and user will see ui's quantiles in Web UI. They may think the are same. we need to add this default value to monitoring page too.

@srowen
Copy link
Member

srowen commented Jan 8, 2021

I see, OK. Yes I agree the default should be consistent with the UI, but, it is a minor breaking change, I suppose. I wonder if it's worth it, or needs to wait until Spark 4. Open to dev@ if you like for visibility and comments.

@SparkQA
Copy link

SparkQA commented Jan 8, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38435/

@github-actions github-actions bot added the DOCS label Jan 8, 2021
@SparkQA
Copy link

SparkQA commented Jan 8, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38435/

@SparkQA
Copy link

SparkQA commented Jan 8, 2021

Test build #133846 has finished for PR 31048 at commit ed53e30.

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

@gengliangwang
Copy link
Member

Though it's a minor breaking change, I think it makes more sense to be consistent with the UI and show Min/Max instead of 0.05/0.95 quantiles.

@AngersZhuuuu
Copy link
Contributor Author

Seems got no reply from other dev, emmmm, What should I do next? ping @srowen @gengliangwang @sarutak @tgravescs

@tgravescs
Copy link
Contributor

I'm more on the side of putting it into a 4.0 since it could break someone and if someone doesn't like it they just set what they want.

@AngersZhuuuu
Copy link
Contributor Author

I'm more on the side of putting it into a 4.0 since it could break someone and if someone doesn't like it they just set what they want.

Ok, wait for spark 4.0 release.

@github-actions
Copy link

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!

@github-actions github-actions bot added the Stale label Apr 24, 2021
@github-actions github-actions bot closed this Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants