Skip to content

[SPARK-33287][SS][UI]Expose state custom metrics information on SS UI - #30336

Closed
gaborgsomogyi wants to merge 15 commits into
apache:masterfrom
gaborgsomogyi:SPARK-33287
Closed

[SPARK-33287][SS][UI]Expose state custom metrics information on SS UI#30336
gaborgsomogyi wants to merge 15 commits into
apache:masterfrom
gaborgsomogyi:SPARK-33287

Conversation

@gaborgsomogyi

@gaborgsomogyigaborgsomogyi commented Nov 11, 2020

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Structured Streaming UI is not containing state custom metrics information. In this PR I've added it.

Why are the changes needed?

Missing state custom metrics information.

Does this PR introduce any user-facing change?

Additional UI elements appear.

How was this patch tested?

Existing unit tests + manual test.

#Compile Spark
echo "spark.sql.streaming.ui.enabledCustomMetricList stateOnCurrentVersionSizeBytes" >> conf/spark-defaults.conf
sbin/start-master.sh
sbin/start-worker.sh spark://gsomogyi-MBP16:7077
./bin/spark-submit --master spark://gsomogyi-MBP16:7077 --deploy-mode client --class com.spark.Main ../spark-test/target/spark-test-1.0-SNAPSHOT-jar-with-dependencies.jar

Screenshot 2020-11-18 at 12 45 36

@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

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

@HyukjinKwon

Copy link
Copy Markdown
Member

cc @gengliangwang, @HeartSaVioR and @xuanyuanking FYI

@gaborgsomogyi

gaborgsomogyi commented Nov 11, 2020

Copy link
Copy Markdown
ContributorAuthor

Thanks pinging peoples, just wanted to wait until tests pass.

@SparkQA

Copy link
Copy Markdown

Test build #130939 has finished for PR 30336 at commit 2eefeb2.

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

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated

@xuanyuankingxuanyuanking left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the work, will also test offline! cc @uncleGen@zsxwing

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated
@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

Test build #131074 has finished for PR 30336 at commit 19e2da1.

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

@SparkQA

Copy link
Copy Markdown

Test build #131072 has finished for PR 30336 at commit f11bc22.

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

@SparkQA

Copy link
Copy Markdown

Test build #131064 has finished for PR 30336 at commit 6654934.

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

@HeartSaVioRHeartSaVioR left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One comment on the location of config, otherwise looks OK. Manually verified the functionality as well.

Comment threadcore/src/main/scala/org/apache/spark/internal/config/UI.scala Outdated
@HyukjinKwon

Copy link
Copy Markdown
Member

Oh, @sarutak too FYI

Comment threadcore/src/main/scala/org/apache/spark/internal/config/UI.scala Outdated
@sarutak

sarutak commented Nov 16, 2020

Copy link
Copy Markdown
Member

Hi @gaborgsomogyi , you'd like to add this new feature but you say you tested this feature manually so could you explain how you tested to us in the description? It helps reviewers.

@gaborgsomogyi

Copy link
Copy Markdown
ContributorAuthor

@sarutak added how I've tested.

@sarutak

sarutak commented Nov 16, 2020

Copy link
Copy Markdown
Member

@gaborgsomogyi I expected what the streaming query is (code snippet may be enough) and what you confirmed by the manual test... With those information, we can understand what you did (and did not) test.
Here is quoted from the PR template.

If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.

@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

Test build #131349 has finished for PR 30336 at commit d43b1b9.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class StateStoreCustomSumMetric(name: String, desc: String, override val unit: String)
  • case class StateStoreCustomSizeMetric(name: String, desc: String, override val unit: String)
  • case class StateStoreCustomTimingMetric(name: String, desc: String, override val unit: String)

@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

Test build #131412 has finished for PR 30336 at commit 9a87255.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class StateStoreCustomSumMetric(name: String, desc: String) extends StateStoreCustomMetric
  • case class StateStoreCustomSizeMetric(name: String, desc: String) extends StateStoreCustomMetric
  • case class StateStoreCustomTimingMetric(name: String, desc: String) extends StateStoreCustomMetric

@HeartSaVioR

Copy link
Copy Markdown
Contributor

@gaborgsomogyi Could you please fix the merge conflict?

@xuanyuanking
This PR reverted the API change which would address your review comment. Could you please verify on your own? It's appreciated if you have more comments, but I see this was the only concern.
#30336 (comment)

Once merge conflict is fixed, I'll merge in early next week unless there's no further comment.

@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

Test build #131463 has finished for PR 30336 at commit 864062f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class GetShufflePushMergerLocations(numMergersNeeded: Int, hostsToFilter: Set[String])
  • case class RemoveShufflePushMergerLocation(host: String) extends ToBlockManagerMaster
  • case class ParseUrl(children: Seq[Expression], failOnError: Boolean = SQLConf.get.ansiEnabled)

@xuanyuanking

Copy link
Copy Markdown
Member

LGTM, @HeartSaVioR thanks for pinging me!

@SparkQA

Copy link
Copy Markdown

Test build #131657 has finished for PR 30336 at commit 115de28.

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

@gaborgsomogyi

Copy link
Copy Markdown
ContributorAuthor

retest this please

@SparkQA

Copy link
Copy Markdown

Test build #131676 has finished for PR 30336 at commit 115de28.

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

@SparkQA

Copy link
Copy Markdown

Test build #131690 has started for PR 30336 at commit e2f1594.

@HeartSaVioR

Copy link
Copy Markdown
Contributor

I'll merge this as Github Action passed for the last change (e2f1594). Jenkins seems to have been unstable and I don't like to drag it anymore due to this.

@HeartSaVioR

Copy link
Copy Markdown
Contributor

Thanks all for reviewing and thanks @gaborgsomogyi for the contribution. Merged to master.

@gaborgsomogyi

Copy link
Copy Markdown
ContributorAuthor

Thank you all for taking care!

Sign up for freeto 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.

8 participants

@gaborgsomogyi@SparkQA@HyukjinKwon@sarutak@HeartSaVioR@xuanyuanking@viirya@zsxwing