[GitHub] spark pull request #21644: [SPARK-24660][SHS] Show correct error pages when ...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/21644 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21644: [SPARK-24660][SHS] Show correct error pages when ...
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21644#discussion_r198598085
--- Diff:
core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala
---
@@ -140,11 +140,9 @@ private[v1] class AbstractApplicationResource extends
BaseAppResource {
.header("Content-Type", MediaType.APPLICATION_OCTET_STREAM)
.build()
} catch {
- case NonFatal(e) =>
-Response.serverError()
- .entity(s"Event logs are not available for app: $appId.")
- .status(Response.Status.SERVICE_UNAVAILABLE)
- .build()
+ case NonFatal(_) =>
+UIUtils.buildErrorResponse(Response.Status.SERVICE_UNAVAILABLE,
--- End diff --
oh, now I see what you mean, sorry, I didn't get it. I am doing that,
thanks.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21644: [SPARK-24660][SHS] Show correct error pages when ...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21644#discussion_r198586841
--- Diff:
core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala
---
@@ -140,11 +140,9 @@ private[v1] class AbstractApplicationResource extends
BaseAppResource {
.header("Content-Type", MediaType.APPLICATION_OCTET_STREAM)
.build()
} catch {
- case NonFatal(e) =>
-Response.serverError()
- .entity(s"Event logs are not available for app: $appId.")
- .status(Response.Status.SERVICE_UNAVAILABLE)
- .build()
+ case NonFatal(_) =>
+UIUtils.buildErrorResponse(Response.Status.SERVICE_UNAVAILABLE,
--- End diff --
But isn't the type being set in the exception now? Seems like the same
thing that happens in other cases, where the response is set to json but the
exception overrides it when thrown.
But if it doesn't really work, then ok.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21644: [SPARK-24660][SHS] Show correct error pages when ...
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21644#discussion_r198585141
--- Diff:
core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala
---
@@ -140,11 +140,9 @@ private[v1] class AbstractApplicationResource extends
BaseAppResource {
.header("Content-Type", MediaType.APPLICATION_OCTET_STREAM)
.build()
} catch {
- case NonFatal(e) =>
-Response.serverError()
- .entity(s"Event logs are not available for app: $appId.")
- .status(Response.Status.SERVICE_UNAVAILABLE)
- .build()
+ case NonFatal(_) =>
+UIUtils.buildErrorResponse(Response.Status.SERVICE_UNAVAILABLE,
--- End diff --
no, we need to set the type, otherwise it returns the response as
octet-stream which is causing the issue..
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21644: [SPARK-24660][SHS] Show correct error pages when ...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21644#discussion_r198574641
--- Diff:
core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala
---
@@ -140,11 +140,9 @@ private[v1] class AbstractApplicationResource extends
BaseAppResource {
.header("Content-Type", MediaType.APPLICATION_OCTET_STREAM)
.build()
} catch {
- case NonFatal(e) =>
-Response.serverError()
- .entity(s"Event logs are not available for app: $appId.")
- .status(Response.Status.SERVICE_UNAVAILABLE)
- .build()
+ case NonFatal(_) =>
+UIUtils.buildErrorResponse(Response.Status.SERVICE_UNAVAILABLE,
--- End diff --
This could just `throw ServiceUnavailable`, no?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21644: [SPARK-24660][SHS] Show correct error pages when ...
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21644#discussion_r198562109
--- Diff:
core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala ---
@@ -148,38 +148,21 @@ private[v1] trait BaseAppResource extends
ApiRequestContext {
}
private[v1] class ForbiddenException(msg: String) extends
WebApplicationException(
- Response.status(Response.Status.FORBIDDEN).entity(msg).build())
+UIUtils.buildErrorResponse(Response.Status.FORBIDDEN, msg))
private[v1] class NotFoundException(msg: String) extends
WebApplicationException(
- new NoSuchElementException(msg),
-Response
- .status(Response.Status.NOT_FOUND)
- .entity(ErrorWrapper(msg))
- .build()
-)
+new NoSuchElementException(msg),
+UIUtils.buildErrorResponse(Response.Status.NOT_FOUND, msg))
private[v1] class ServiceUnavailable(msg: String) extends
WebApplicationException(
- new ServiceUnavailableException(msg),
- Response
-.status(Response.Status.SERVICE_UNAVAILABLE)
-.entity(ErrorWrapper(msg))
-.build()
-)
+new ServiceUnavailableException(msg),
--- End diff --
I agree, I updated with your suggestion, thanks
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21644: [SPARK-24660][SHS] Show correct error pages when ...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21644#discussion_r198561212
--- Diff:
core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala ---
@@ -148,38 +148,21 @@ private[v1] trait BaseAppResource extends
ApiRequestContext {
}
private[v1] class ForbiddenException(msg: String) extends
WebApplicationException(
- Response.status(Response.Status.FORBIDDEN).entity(msg).build())
+UIUtils.buildErrorResponse(Response.Status.FORBIDDEN, msg))
private[v1] class NotFoundException(msg: String) extends
WebApplicationException(
- new NoSuchElementException(msg),
-Response
- .status(Response.Status.NOT_FOUND)
- .entity(ErrorWrapper(msg))
- .build()
-)
+new NoSuchElementException(msg),
+UIUtils.buildErrorResponse(Response.Status.NOT_FOUND, msg))
private[v1] class ServiceUnavailable(msg: String) extends
WebApplicationException(
- new ServiceUnavailableException(msg),
- Response
-.status(Response.Status.SERVICE_UNAVAILABLE)
-.entity(ErrorWrapper(msg))
-.build()
-)
+new ServiceUnavailableException(msg),
--- End diff --
I think it's ok to leave (almost) as is. It avoid duplicating the status
code in every call, which would be annoying.
But we can remove the "cause" exceptions from all these wrappers since
they're redundant.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21644: [SPARK-24660][SHS] Show correct error pages when ...
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21644#discussion_r198560296
--- Diff:
core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala ---
@@ -148,38 +148,21 @@ private[v1] trait BaseAppResource extends
ApiRequestContext {
}
private[v1] class ForbiddenException(msg: String) extends
WebApplicationException(
- Response.status(Response.Status.FORBIDDEN).entity(msg).build())
+UIUtils.buildErrorResponse(Response.Status.FORBIDDEN, msg))
private[v1] class NotFoundException(msg: String) extends
WebApplicationException(
- new NoSuchElementException(msg),
-Response
- .status(Response.Status.NOT_FOUND)
- .entity(ErrorWrapper(msg))
- .build()
-)
+new NoSuchElementException(msg),
+UIUtils.buildErrorResponse(Response.Status.NOT_FOUND, msg))
private[v1] class ServiceUnavailable(msg: String) extends
WebApplicationException(
- new ServiceUnavailableException(msg),
- Response
-.status(Response.Status.SERVICE_UNAVAILABLE)
-.entity(ErrorWrapper(msg))
-.build()
-)
+new ServiceUnavailableException(msg),
--- End diff --
yes, indeed they do. Not too much code but
```
throw new ForbiddenException(raw"""user "$user" is not authorized""")
```
would become
```
throw new ForbiddenException(UIUtils.buildErrorResponse(
Response.Status.FORBIDDEN, raw"""user "$user" is not
authorized"""))
```
what do you think? Shall we do the refactor?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21644: [SPARK-24660][SHS] Show correct error pages when ...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21644#discussion_r198558986
--- Diff:
core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala ---
@@ -148,38 +148,21 @@ private[v1] trait BaseAppResource extends
ApiRequestContext {
}
private[v1] class ForbiddenException(msg: String) extends
WebApplicationException(
- Response.status(Response.Status.FORBIDDEN).entity(msg).build())
+UIUtils.buildErrorResponse(Response.Status.FORBIDDEN, msg))
private[v1] class NotFoundException(msg: String) extends
WebApplicationException(
- new NoSuchElementException(msg),
-Response
- .status(Response.Status.NOT_FOUND)
- .entity(ErrorWrapper(msg))
- .build()
-)
+new NoSuchElementException(msg),
+UIUtils.buildErrorResponse(Response.Status.NOT_FOUND, msg))
private[v1] class ServiceUnavailable(msg: String) extends
WebApplicationException(
- new ServiceUnavailableException(msg),
- Response
-.status(Response.Status.SERVICE_UNAVAILABLE)
-.entity(ErrorWrapper(msg))
-.build()
-)
+new ServiceUnavailableException(msg),
--- End diff --
Hmm, if that's the case then probably the wrappers can save some code
(since they abstract the building of the response)...
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21644: [SPARK-24660][SHS] Show correct error pages when ...
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21644#discussion_r198554094
--- Diff:
core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala ---
@@ -148,38 +148,21 @@ private[v1] trait BaseAppResource extends
ApiRequestContext {
}
private[v1] class ForbiddenException(msg: String) extends
WebApplicationException(
- Response.status(Response.Status.FORBIDDEN).entity(msg).build())
+UIUtils.buildErrorResponse(Response.Status.FORBIDDEN, msg))
private[v1] class NotFoundException(msg: String) extends
WebApplicationException(
- new NoSuchElementException(msg),
-Response
- .status(Response.Status.NOT_FOUND)
- .entity(ErrorWrapper(msg))
- .build()
-)
+new NoSuchElementException(msg),
+UIUtils.buildErrorResponse(Response.Status.NOT_FOUND, msg))
private[v1] class ServiceUnavailable(msg: String) extends
WebApplicationException(
- new ServiceUnavailableException(msg),
- Response
-.status(Response.Status.SERVICE_UNAVAILABLE)
-.entity(ErrorWrapper(msg))
-.build()
-)
+new ServiceUnavailableException(msg),
--- End diff --
thanks for your review. Sure I can do the refactor, but the helper method
would be needed anyway. Indeed, the problem which was present before the PR is
that we are not specifying the type of the response, so it takes the type which
is produced. And in the default exceptions you mentioned the `Response` is
built without setting the type, so we still need to pass them the `Response` we
build in the helper method.
I will follow your suggestion (so I'll clean up our exceptions using the
jax ones), but keeping the helper method for this reason. Thanks.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21644: [SPARK-24660][SHS] Show correct error pages when ...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21644#discussion_r198548600
--- Diff:
core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala
---
@@ -140,11 +140,9 @@ private[v1] class AbstractApplicationResource extends
BaseAppResource {
.header("Content-Type", MediaType.APPLICATION_OCTET_STREAM)
.build()
} catch {
- case NonFatal(e) =>
-Response.serverError()
- .entity(s"Event logs are not available for app: $appId.")
- .status(Response.Status.SERVICE_UNAVAILABLE)
- .build()
+ case NonFatal(_) =>
+UIUtils.buildErrorResponse(Response.Status.SERVICE_UNAVAILABLE,
--- End diff --
If doing the cleanup, it's probably ok to just throw the exception here.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21644: [SPARK-24660][SHS] Show correct error pages when ...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21644#discussion_r198548363
--- Diff:
core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala ---
@@ -148,38 +148,21 @@ private[v1] trait BaseAppResource extends
ApiRequestContext {
}
private[v1] class ForbiddenException(msg: String) extends
WebApplicationException(
- Response.status(Response.Status.FORBIDDEN).entity(msg).build())
+UIUtils.buildErrorResponse(Response.Status.FORBIDDEN, msg))
private[v1] class NotFoundException(msg: String) extends
WebApplicationException(
- new NoSuchElementException(msg),
-Response
- .status(Response.Status.NOT_FOUND)
- .entity(ErrorWrapper(msg))
- .build()
-)
+new NoSuchElementException(msg),
+UIUtils.buildErrorResponse(Response.Status.NOT_FOUND, msg))
private[v1] class ServiceUnavailable(msg: String) extends
WebApplicationException(
- new ServiceUnavailableException(msg),
- Response
-.status(Response.Status.SERVICE_UNAVAILABLE)
-.entity(ErrorWrapper(msg))
-.build()
-)
+new ServiceUnavailableException(msg),
--- End diff --
Hmm... none of this is your fault, but it seems these exceptions are not
needed anymore. jax-rs 2.1 has exceptions for all these and they could just
replace these wrappers.
e.g. if you just throw `ServiceUnavailableException` directly instead of
this wrapper, the result would be the same.
You can find all exceptions at:
https://jax-rs.github.io/apidocs/2.1/javax/ws/rs/package-summary.html
I think it's better to clean this up instead of adding another helper API.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21644: [SPARK-24660][SHS] Show correct error pages when ...
Github user attilapiros commented on a diff in the pull request:
https://github.com/apache/spark/pull/21644#discussion_r198185350
--- Diff:
core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala ---
@@ -148,38 +148,36 @@ private[v1] trait BaseAppResource extends
ApiRequestContext {
}
private[v1] class ForbiddenException(msg: String) extends
WebApplicationException(
- Response.status(Response.Status.FORBIDDEN).entity(msg).build())
+
Response.status(Response.Status.FORBIDDEN).entity(msg).`type`(MediaType.TEXT_PLAIN).build())
--- End diff --
You can extract a new helper function to build the response object from a
`Response.Status` and a message. I think UIUtils is good place for such a
function then you can use it at `OneApplicationResource.scala` too as the
`serverError`method just sets the status to `Status.INTERNAL_SERVER_ERROR`
which overwritten by right away with `Response.Status.SERVICE_UNAVAILABLE`.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21644: [SPARK-24660][SHS] Show correct error pages when ...
GitHub user mgaido91 opened a pull request: https://github.com/apache/spark/pull/21644 [SPARK-24660][SHS] Show correct error pages when downloading logs ## What changes were proposed in this pull request? SHS is showing bad errors when trying to download logs is not successful. This may happen because the requested application doesn't exist or the user doesn't have permissions for it, for instance. The PR fixes the response when errors occur, so that they are displayed properly. ## How was this patch tested? manual tests **Before the patch:** 1. Unauthorized user  2. Non-existing application  **After the patch** 1. Unauthorized user  2. Non-existing application  You can merge this pull request into a Git repository by running: $ git pull https://github.com/mgaido91/spark SPARK-24660 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/21644.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #21644 commit 8b26a5359e9d759668d6608ebf13952d4fa088c7 Author: Marco Gaido Date: 2018-06-26T14:01:28Z [SPARK-24660][SHS] Show correct error pages when downloading logs --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
