You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "kuwii (via GitHub)" <gi...@apache.org> on 2023/11/03 05:06:58 UTC
[PR] [SPARK-45556][UI] Allow customized status code and message through WebApplicationException [spark]
kuwii opened a new pull request, #43646:
URL: https://github.com/apache/spark/pull/43646
### What changes were proposed in this pull request?
Adds an extra catch of `WebApplicationException` in Spark core and history server's web page servlet, to let customized response to pass-through instead of wrapping them in a 500 response, to make web page and REST API have consistent behavior.
After the change, when a `WebApplicationException` is thrown, servlet will render a basic Spark page with the exception message, and respond with the status code of the exception.
### Why are the changes needed?
Currently for web page servlet in Spark, when an exception is thrown, there's only 3 possible responses:
- If it is history server unable to find an app, a 404 will be responded.
- It the exception is an `IllegalArgumentException`, a 400 will be responded.
- Otherwise, a 500 will be responded, wrapping the original exception within it.
However for REST API, if a response is a `WebApplicationException`, its response can be directly returned with the designed status code and message, without being wrapped. This causes inconsistent behavior between web page and REST API.
For history server, Spark allows to use alternative `ApplicationHistoryProvider`, or adds extra page and API through "AppHistoryServerPlugin". Suppose some extra provider or plugins is used, and in some cases they want to return a special status code due to their needs. Currently there's no way to do it other than forking Spark because web page servlet does not support it.
### Does this PR introduce _any_ user-facing change?
By default, no. Because Spark UI's built-in web page servlet does not throw `WebApplicationException`.
For 3rd party provider or plugin, if a `WebApplicationException` is thrown, after this change, its message and status code can be directly responded. Previously it is wrapped within a 500 response.
### How was this patch tested?
- 3 UTs are added:
- Testing whether history server servlet can handle `WebApplicationException`
- Testing whether servlet created by `JettyUtils` can handle `WebApplicationException`
- Testing whether servlet created by `JettyUtils` can handle other exceptions as before
- Manually tested
### Was this patch authored or co-authored using generative AI tooling?
No
--
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-45556][UI] Allow web page respond customized status code and message through WebApplicationException [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #43646: [SPARK-45556][UI] Allow web page respond customized status code and message through WebApplicationException
URL: https://github.com/apache/spark/pull/43646
--
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-45556][UI] Allow web page respond customized status code and message through WebApplicationException [spark]
Posted by "kuwii (via GitHub)" <gi...@apache.org>.
kuwii commented on code in PR #43646:
URL: https://github.com/apache/spark/pull/43646#discussion_r1404701377
##########
core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala:
##########
@@ -93,16 +95,23 @@ abstract class HistoryServerSuite extends SparkFunSuite with BeforeAndAfter with
.set(EXECUTOR_PROCESS_TREE_METRICS_ENABLED, true)
.set(HYBRID_STORE_DISK_BACKEND, diskBackend.toString)
conf.setAll(extraConf)
- provider = new FsHistoryProvider(conf)
- provider.checkForLogs()
+ val usedProvider = appHistoryProvider.getOrElse {
Review Comment:
Yes, to test whether history server returns correct response, the UT added in this file requires to create history server service with a private implementation of provider interface, to throw customized error. But current version only creates `FsHistoryProvider`.
So change here is to allow to pass private provider to initialization method. The previous init way is still kept, so it won't affect other existing tests.
--
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-45556][UI] Allow web page respond customized status code and message through WebApplicationException [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #43646:
URL: https://github.com/apache/spark/pull/43646#issuecomment-2002201536
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!
--
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-45556][UI] Allow web page respond customized status code and message through WebApplicationException [spark]
Posted by "kuwii (via GitHub)" <gi...@apache.org>.
kuwii commented on PR #43646:
URL: https://github.com/apache/spark/pull/43646#issuecomment-1793488046
> Would be great if we can contain the screenshot of UI with this fix.
Thanks for the comment. Have updated the screenshot before and after the change.
--
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-45556][UI] Allow web page respond customized status code and message through WebApplicationException [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43646:
URL: https://github.com/apache/spark/pull/43646#issuecomment-1793280427
Would be great if we can contain the screenshot of UI with this fix.
--
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-45556][UI] Allow web page respond customized status code and message through WebApplicationException [spark]
Posted by "kuwii (via GitHub)" <gi...@apache.org>.
kuwii commented on PR #43646:
URL: https://github.com/apache/spark/pull/43646#issuecomment-1846300071
Kindly ping @srowen
--
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-45556][UI] Allow web page respond customized status code and message through WebApplicationException [spark]
Posted by "kuwii (via GitHub)" <gi...@apache.org>.
kuwii commented on code in PR #43646:
URL: https://github.com/apache/spark/pull/43646#discussion_r1404701377
##########
core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala:
##########
@@ -93,16 +95,23 @@ abstract class HistoryServerSuite extends SparkFunSuite with BeforeAndAfter with
.set(EXECUTOR_PROCESS_TREE_METRICS_ENABLED, true)
.set(HYBRID_STORE_DISK_BACKEND, diskBackend.toString)
conf.setAll(extraConf)
- provider = new FsHistoryProvider(conf)
- provider.checkForLogs()
+ val usedProvider = appHistoryProvider.getOrElse {
Review Comment:
Yes, to test whether history server returnscorrect response, the UT added in this file requires to create history server service with a private implementation of provider interface, to throw customized error. But current version only creates `FsHistoryProvider`.
So change here is to allow to create pass private provider to initialization method. The previous init way is still kept, so it won't affect other existing tests.
##########
core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala:
##########
@@ -93,16 +95,23 @@ abstract class HistoryServerSuite extends SparkFunSuite with BeforeAndAfter with
.set(EXECUTOR_PROCESS_TREE_METRICS_ENABLED, true)
.set(HYBRID_STORE_DISK_BACKEND, diskBackend.toString)
conf.setAll(extraConf)
- provider = new FsHistoryProvider(conf)
- provider.checkForLogs()
+ val usedProvider = appHistoryProvider.getOrElse {
Review Comment:
Yes, to test whether history server returns correct response, the UT added in this file requires to create history server service with a private implementation of provider interface, to throw customized error. But current version only creates `FsHistoryProvider`.
So change here is to allow to create pass private provider to initialization method. The previous init way is still kept, so it won't affect other existing tests.
--
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-45556][UI] Allow web page respond customized status code and message through WebApplicationException [spark]
Posted by "kuwii (via GitHub)" <gi...@apache.org>.
kuwii commented on code in PR #43646:
URL: https://github.com/apache/spark/pull/43646#discussion_r1404701377
##########
core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala:
##########
@@ -93,16 +95,23 @@ abstract class HistoryServerSuite extends SparkFunSuite with BeforeAndAfter with
.set(EXECUTOR_PROCESS_TREE_METRICS_ENABLED, true)
.set(HYBRID_STORE_DISK_BACKEND, diskBackend.toString)
conf.setAll(extraConf)
- provider = new FsHistoryProvider(conf)
- provider.checkForLogs()
+ val usedProvider = appHistoryProvider.getOrElse {
Review Comment:
Yes, to test whether history server return correct response, the UT added in this file requires to create history server service with a private implementation of provider interface, to throw customized error. But current version only creates `FsHistoryProvider`.
So change here is to allow to create pass private provider to initialization method. The previous init way is still kept, so it won't affect other existing tests.
--
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-45556][UI] Allow web page respond customized status code and message through WebApplicationException [spark]
Posted by "kuwii (via GitHub)" <gi...@apache.org>.
kuwii commented on PR #43646:
URL: https://github.com/apache/spark/pull/43646#issuecomment-1826186284
> Seems OK. It's a little bit better than the raw stack trace. I don't suppose there's a similar change to make in the main Spark UI? or does it also already work this way?
I don't think Spark's built-in UI needs this change, because web page doesn't seem to throw `WebApplicationException`, and REST API already supports this. The gap may be because web page and REST API are using different ways to handle HTTP requests.
The main goal of this PR is to let web page part also able to return customized response code (like 418 as an example), instead of just 200, 400, 404 and 500. It is basically for those who wants to extend Spark UI through plugins, without forking Spark source code.
--
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-45556][UI] Allow web page respond customized status code and message through WebApplicationException [spark]
Posted by "kuwii (via GitHub)" <gi...@apache.org>.
kuwii commented on PR #43646:
URL: https://github.com/apache/spark/pull/43646#issuecomment-1803202085
Kindly ping @HyukjinKwon . Could you please help to take a look at this PR if you think it is OK to do this change? Thanks.
--
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-45556][UI] Allow web page respond customized status code and message through WebApplicationException [spark]
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on code in PR #43646:
URL: https://github.com/apache/spark/pull/43646#discussion_r1402078652
##########
core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala:
##########
@@ -93,16 +95,23 @@ abstract class HistoryServerSuite extends SparkFunSuite with BeforeAndAfter with
.set(EXECUTOR_PROCESS_TREE_METRICS_ENABLED, true)
.set(HYBRID_STORE_DISK_BACKEND, diskBackend.toString)
conf.setAll(extraConf)
- provider = new FsHistoryProvider(conf)
- provider.checkForLogs()
+ val usedProvider = appHistoryProvider.getOrElse {
Review Comment:
Is this a related change?
--
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-45556][UI] Allow web page respond customized status code and message through WebApplicationException [spark]
Posted by "kuwii (via GitHub)" <gi...@apache.org>.
kuwii commented on PR #43646:
URL: https://github.com/apache/spark/pull/43646#issuecomment-1822106783
Also kindly ping @srowen @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