You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dongjoon-hyun (via GitHub)" <gi...@apache.org> on 2023/11/03 03:27:34 UTC
[PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
dongjoon-hyun opened a new pull request, #43643:
URL: https://github.com/apache/spark/pull/43643
…
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
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.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381201021
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
If the user sets a different value for `spark.eventLog.dir` from the default value
which `spark.ui.historyServerUrl` points to, he/she might get confused while not able to find their apps
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.master.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43643:
URL: https://github.com/apache/spark/pull/43643#issuecomment-1793219842
Thank you, @GabeChurch .
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.master.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn closed pull request #43643: [SPARK-45774][CORE][UI] Support `spark.master.ui.historyServerUrl` in `ApplicationPage`
URL: https://github.com/apache/spark/pull/43643
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381207553
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
- To be clear, I agree with you that your point is valid and reasonable in a single large shared cluster like YARN.
- However, for small team-owned small clusters which belong to a single K8s cluster, the use cases and requirements are quite different what you assume.
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381206010
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
It seems that I found what was the conflicts here. For the following, actually, we have a team-wide small Spark cluster, not a giant one you are supposed here, @yaooqinn .
> Setting spark.eventLog.dir/spark.ui.historyServerUrl in pairs is practical, for example,
> A user wants to keep its runtime data private
> The public history server is frequently overloaded, so they require a custom one with improved UX.
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #43643:
URL: https://github.com/apache/spark/pull/43643#issuecomment-1791972092
Thank you for the reference and detailed explanation @dongjoon-hyun.
Adding extra hints for users looks good to me.
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381212592
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
How about we get this address from the user's app first, and if not, then from the standalone master's config?
It looks like meets the requirements for both large and small deployments.
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381204888
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
I understand what's your point.
- However, in the production environments, SRE or submission proxy enforces it by rules in general.
- IIUC, `spark.yarn.historyServer.address` has the same issue because it depends on runtime YARN environment substitution. Did I misunderstand?
https://github.com/apache/spark/blob/b9d379a6b84b67b29ccb578938764b888d64f293/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala#L946-L955
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381207553
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
- To be clear, I agree with you that your point is valid and reasonable in a single large shared cluster like YARN.
- However, for small team-owned small clusters which belong to a single K8s cluster, the use cases and requirements are quite different from what you assume.
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.master.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43643:
URL: https://github.com/apache/spark/pull/43643#issuecomment-1791990923
According to your advice,
- I revised the config description and mentioned the assumption explicitly.
- Also, I changed the namespace of configuration from `spark.ui.historyServerUrl` to `spark.master.ui.historyServerUrl`.
Thank you again!
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381217165
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
For this one, I can file a JIRA for that.
> How about we get this address from the user's app first, and if not, then from the standalone master's config?
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.master.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #43643:
URL: https://github.com/apache/spark/pull/43643#issuecomment-1792005808
We can add it to the [ApplicationDescription](https://github.com/apache/spark/blob/b9d379a6b84b67b29ccb578938764b888d64f293/core/src/main/scala/org/apache/spark/deploy/ApplicationDescription.scala#L24-L36) ahead when you meet such a use case in the future:)
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381193777
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
> app knows only spark.eventLog.dir, not SHS info
Not really. There is already an existing behavior for Spark on Yarn. We can set the history server via `spark.yarn.historyServer.address`
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381161116
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
The history server is configured application by application, not spark master. So, IMO, we shall get the history server from the app itself instead.
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381206010
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
It seems that I found what was the conflicts here. For the following, actually, we have a team-wide small Spark cluster, not a giant one you are supposed here.
> Setting spark.eventLog.dir/spark.ui.historyServerUrl in pairs is practical, for example,
> A user wants to keep its runtime data private
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
It seems that I found what was the conflicts here. For the following, actually, we have a team-wide small Spark cluster, not a giant one you are supposed here, @yaooqinn .
> Setting spark.eventLog.dir/spark.ui.historyServerUrl in pairs is practical, for example,
> A user wants to keep its runtime data private
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381189549
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
Thank you for review, @yaooqinn .
That's the reason why this is described as the best effort approach which depends on the user's setting.
However, technically, your suggestion is not true, @yaooqinn , because SHS is orthogonal from apps too. For example, app knows only `spark.eventLog.dir`, not SHS info. In addition, you can launch multiple SHS based on the same event location.
> So, IMO, we shall get the history server from the app itself instead.
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381199271
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
This PR is a general solution and allows the user to provide their YARN URL to this new `spark.ui.historyServerUrl` too. I don't think this PR rejects the existing conf.
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381206010
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
It seems that I found what was the conflicts between you and me here. For the following, actually, we have a team-wide small Spark cluster, not a giant one you are supposed here, @yaooqinn .
> Setting spark.eventLog.dir/spark.ui.historyServerUrl in pairs is practical, for example,
> A user wants to keep its runtime data private
> The public history server is frequently overloaded, so they require a custom one with improved UX.
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381207553
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
- To be clear, I agree with you that your point is valid and reasonable in a single large shared cluster.
- However, for small team-owned small clusters which belong to a single K8s cluster, the use cases and requirements are quite different what you assume.
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43643:
URL: https://github.com/apache/spark/pull/43643#issuecomment-1791863587
Could you review this, @LuciferYang ?
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381201021
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
If the user sets a different value for `spark.eventLog.dir` from the default value
which `spark.ui.historyServerUrl` points to, he/she might get confused while not able to find their apps.
Setting `spark.eventLog.dir/spark.ui.historyServerUrl` in pairs is practical, for example,
- A user wants to keep its runtime data private
- The public history server is frequently overloaded, so they require a custom one with improved UX.
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381209229
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
Only variables(spark.yarn.historyServer.address) that match the ${hadoopconf- .. } pattern are substituted. Otherwise, it's an identity operation.
Besides, `yarnConf` is client-side prioritized.
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43643:
URL: https://github.com/apache/spark/pull/43643#issuecomment-1791961207
To @yaooqinn ,
- Although I added two above comments, [1st](https://github.com/apache/spark/pull/43643#discussion_r1381206010) and [2nd](https://github.com/apache/spark/pull/43643#discussion_r1381207553), I believe I didn't explain the purpose clearly. It's my bad.
- I've been working in the context of the umbrella issue, [SPARK-45756 Revisit and Improve Spark Standalone Cluster](https://issues.apache.org/jira/browse/SPARK-45756), for mainly on Spark Standalone cluster on K8s environment. For your concerns, I can add clearly the **assumption**, **limitation**, **use cases** description to this new configuration. For example, I can mention that `Don't use this with YARN environment or YARN users`.
I hope this new feature is able to co-exist without causesing user confusion. WDTY?
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381217165
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
For this one, I can file a JIRA for that YARN-specific feature.
> How about we get this address from the user's app first, and if not, then from the standalone master's config?
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.master.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #43643:
URL: https://github.com/apache/spark/pull/43643#issuecomment-1791993934
> BTW, may I ask why you think ApplicationPage can get that information from App after completion?
Just like other fields that used to describe the app after termination?
---
Thank you for the new name!
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.master.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43643:
URL: https://github.com/apache/spark/pull/43643#issuecomment-1792005950
Anyway, I created the following JIRA based on the above discussion
- SPARK-45778 Get SHS URL from Spark applications
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381197465
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
Well, it's YARN-specific thing. I don't think that could be general in this era.
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.master.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "GabeChurch (via GitHub)" <gi...@apache.org>.
GabeChurch commented on PR #43643:
URL: https://github.com/apache/spark/pull/43643#issuecomment-1793054890
This is a great feature addition that has long been missing. Thanks @dongjoon-hyun
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.master.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43643:
URL: https://github.com/apache/spark/pull/43643#issuecomment-1792001273
Thank you for approval.
`Spark Master`'s `ApplicationPage` only have `ApplicationInfo` (and `ApplicationDescription`). That's all Spark has. We don't have any further information like `ApplicationMaster` (of `yarn` module).
https://github.com/apache/spark/blob/b9d379a6b84b67b29ccb578938764b888d64f293/core/src/main/scala/org/apache/spark/deploy/master/ApplicationInfo.scala#L31-L37
https://github.com/apache/spark/blob/b9d379a6b84b67b29ccb578938764b888d64f293/core/src/main/scala/org/apache/spark/deploy/ApplicationDescription.scala#L24-L36
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.master.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43643:
URL: https://github.com/apache/spark/pull/43643#issuecomment-1792634301
Thank you, @yaooqinn !
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.master.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #43643:
URL: https://github.com/apache/spark/pull/43643#issuecomment-1792625989
Thank you, @dongjoon-hyun
Merged to master
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43643:
URL: https://github.com/apache/spark/pull/43643#issuecomment-1791974206
Thank you so much.
BTW, may I ask why you think `ApplicationPage` can get that information from `App` after completion? I want to leave a note to the new JIRA for YARN. From my understanding, `ApplicationPage` seems to be isolated from that YARN `ApplicatoinMaster` code part.
--
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
Re: [PR] [SPARK-45774][CORE][UI] Support `spark.ui.historyServerUrl` in `ApplicationPage` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43643:
URL: https://github.com/apache/spark/pull/43643#discussion_r1381189549
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala:
##########
@@ -98,6 +98,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
<a href={UIUtils.makeHref(parent.master.reverseProxy,
app.id, app.desc.appUiUrl)}>Application Detail UI</a>
</strong></li>
+ } else if (parent.master.historyServerUrl.nonEmpty) {
+ <li><strong>
+ <a href={s"${parent.master.historyServerUrl.get}/history/${app.id}"}>
Review Comment:
Thank you for review, @yaooqinn .
That's the reason why this is described as the best effort approach which depends on the user's setting.
However, technically, your suggestion is not true, @yaooqinn , because SHS is orthogonal from apps too. For example, app knows only `spark.eventLog.dir`, not SHS info. In addition, you can launch multiple SHS based on the same event location. Lastly, SHS can be behind proxy too. So, only the users know the exposed SHS URL.
> So, IMO, we shall get the history server from the app itself instead.
--
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