From 8b26a5359e9d759668d6608ebf13952d4fa088c7 Mon Sep 17 00:00:00 2001 From: Marco Gaido Date: Tue, 26 Jun 2018 16:01:28 +0200 Subject: [PATCH 1/6] [SPARK-24660][SHS] Show correct error pages when downloading logs --- .../spark/status/api/v1/ApiRootResource.scala | 18 ++++++++---------- .../status/api/v1/JacksonMessageWriter.scala | 5 +---- .../status/api/v1/OneApplicationResource.scala | 3 ++- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala b/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala index d121068718b8a..74c8c304007c4 100644 --- a/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala +++ b/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala @@ -20,7 +20,7 @@ import java.util.zip.ZipOutputStream import javax.servlet.ServletContext import javax.servlet.http.HttpServletRequest import javax.ws.rs._ -import javax.ws.rs.core.{Context, Response} +import javax.ws.rs.core.{Context, MediaType, Response} import org.eclipse.jetty.server.handler.ContextHandler import org.eclipse.jetty.servlet.{ServletContextHandler, ServletHolder} @@ -148,13 +148,14 @@ 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()) private[v1] class NotFoundException(msg: String) extends WebApplicationException( new NoSuchElementException(msg), Response .status(Response.Status.NOT_FOUND) - .entity(ErrorWrapper(msg)) + .entity(msg) + .`type`(MediaType.TEXT_PLAIN) .build() ) @@ -162,7 +163,8 @@ private[v1] class ServiceUnavailable(msg: String) extends WebApplicationExceptio new ServiceUnavailableException(msg), Response .status(Response.Status.SERVICE_UNAVAILABLE) - .entity(ErrorWrapper(msg)) + .entity(msg) + .`type`(MediaType.TEXT_PLAIN) .build() ) @@ -170,7 +172,8 @@ private[v1] class BadParameterException(msg: String) extends WebApplicationExcep new IllegalArgumentException(msg), Response .status(Response.Status.BAD_REQUEST) - .entity(ErrorWrapper(msg)) + .entity(msg) + .`type`(MediaType.TEXT_PLAIN) .build() ) { def this(param: String, exp: String, actual: String) = { @@ -178,8 +181,3 @@ private[v1] class BadParameterException(msg: String) extends WebApplicationExcep } } -/** - * Signal to JacksonMessageWriter to not convert the message into json (which would result in an - * extra set of quotes). - */ -private[v1] case class ErrorWrapper(s: String) diff --git a/core/src/main/scala/org/apache/spark/status/api/v1/JacksonMessageWriter.scala b/core/src/main/scala/org/apache/spark/status/api/v1/JacksonMessageWriter.scala index 76af33c1a18db..4560d300cb0c8 100644 --- a/core/src/main/scala/org/apache/spark/status/api/v1/JacksonMessageWriter.scala +++ b/core/src/main/scala/org/apache/spark/status/api/v1/JacksonMessageWriter.scala @@ -68,10 +68,7 @@ private[v1] class JacksonMessageWriter extends MessageBodyWriter[Object]{ mediaType: MediaType, multivaluedMap: MultivaluedMap[String, AnyRef], outputStream: OutputStream): Unit = { - t match { - case ErrorWrapper(err) => outputStream.write(err.getBytes(StandardCharsets.UTF_8)) - case _ => mapper.writeValue(outputStream, t) - } + mapper.writeValue(outputStream, t) } override def getSize( diff --git a/core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala b/core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala index 974697890dd03..47de0b9419609 100644 --- a/core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala +++ b/core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala @@ -140,9 +140,10 @@ private[v1] class AbstractApplicationResource extends BaseAppResource { .header("Content-Type", MediaType.APPLICATION_OCTET_STREAM) .build() } catch { - case NonFatal(e) => + case NonFatal(_) => Response.serverError() .entity(s"Event logs are not available for app: $appId.") + .`type`(MediaType.TEXT_PLAIN) .status(Response.Status.SERVICE_UNAVAILABLE) .build() } From 9934d47a76431766681907cb661c18c995e21906 Mon Sep 17 00:00:00 2001 From: Marco Gaido Date: Wed, 27 Jun 2018 11:48:31 +0200 Subject: [PATCH 2/6] refactor adding one method to UIUtils --- .../spark/status/api/v1/ApiRootResource.scala | 32 +++++-------------- .../api/v1/OneApplicationResource.scala | 7 ++-- .../scala/org/apache/spark/ui/UIUtils.scala | 5 +++ 3 files changed, 15 insertions(+), 29 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala b/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala index 74c8c304007c4..bfd9726705b5e 100644 --- a/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala +++ b/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala @@ -26,9 +26,8 @@ import org.eclipse.jetty.server.handler.ContextHandler import org.eclipse.jetty.servlet.{ServletContextHandler, ServletHolder} import org.glassfish.jersey.server.ServerProperties import org.glassfish.jersey.servlet.ServletContainer - import org.apache.spark.SecurityManager -import org.apache.spark.ui.SparkUI +import org.apache.spark.ui.{SparkUI, UIUtils} /** * Main entry point for serving spark application metrics as json, using JAX-RS. @@ -148,34 +147,19 @@ private[v1] trait BaseAppResource extends ApiRequestContext { } private[v1] class ForbiddenException(msg: String) extends WebApplicationException( - Response.status(Response.Status.FORBIDDEN).entity(msg).`type`(MediaType.TEXT_PLAIN).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(msg) - .`type`(MediaType.TEXT_PLAIN) - .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(msg) - .`type`(MediaType.TEXT_PLAIN) - .build() -) + new ServiceUnavailableException(msg), + UIUtils.buildErrorResponse(Response.Status.SERVICE_UNAVAILABLE, msg)) private[v1] class BadParameterException(msg: String) extends WebApplicationException( - new IllegalArgumentException(msg), - Response - .status(Response.Status.BAD_REQUEST) - .entity(msg) - .`type`(MediaType.TEXT_PLAIN) - .build() -) { + new IllegalArgumentException(msg), + UIUtils.buildErrorResponse(Response.Status.BAD_REQUEST, msg)) { def this(param: String, exp: String, actual: String) = { this(raw"""Bad value for parameter "$param". Expected a $exp, got "$actual"""") } diff --git a/core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala b/core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala index 47de0b9419609..1f73272c26807 100644 --- a/core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala +++ b/core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala @@ -141,11 +141,8 @@ private[v1] class AbstractApplicationResource extends BaseAppResource { .build() } catch { case NonFatal(_) => - Response.serverError() - .entity(s"Event logs are not available for app: $appId.") - .`type`(MediaType.TEXT_PLAIN) - .status(Response.Status.SERVICE_UNAVAILABLE) - .build() + UIUtils.buildErrorResponse(Response.Status.SERVICE_UNAVAILABLE, + s"Event logs are not available for app: $appId.") } } diff --git a/core/src/main/scala/org/apache/spark/ui/UIUtils.scala b/core/src/main/scala/org/apache/spark/ui/UIUtils.scala index 5d015b0531ef6..4f5fd7d8d7b07 100644 --- a/core/src/main/scala/org/apache/spark/ui/UIUtils.scala +++ b/core/src/main/scala/org/apache/spark/ui/UIUtils.scala @@ -21,6 +21,7 @@ import java.net.URLDecoder import java.text.SimpleDateFormat import java.util.{Date, Locale, TimeZone} import javax.servlet.http.HttpServletRequest +import javax.ws.rs.core.{MediaType, Response} import scala.util.control.NonFatal import scala.xml._ @@ -566,4 +567,8 @@ private[spark] object UIUtils extends Logging { NEWLINE_AND_SINGLE_QUOTE_REGEX.replaceAllIn(requestParameter, "")) } } + + def buildErrorResponse(status: Response.Status, msg: String): Response = { + Response.status(Response.Status.FORBIDDEN).entity(msg).`type`(MediaType.TEXT_PLAIN).build() + } } From 43efe0fdc30795d03ee78c0258bb72f229bfacc7 Mon Sep 17 00:00:00 2001 From: Marco Gaido Date: Wed, 27 Jun 2018 12:09:01 +0200 Subject: [PATCH 3/6] fix scalastyle --- .../scala/org/apache/spark/status/api/v1/ApiRootResource.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala b/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala index bfd9726705b5e..256555c2a3d52 100644 --- a/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala +++ b/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala @@ -20,12 +20,13 @@ import java.util.zip.ZipOutputStream import javax.servlet.ServletContext import javax.servlet.http.HttpServletRequest import javax.ws.rs._ -import javax.ws.rs.core.{Context, MediaType, Response} +import javax.ws.rs.core.{Context, Response} import org.eclipse.jetty.server.handler.ContextHandler import org.eclipse.jetty.servlet.{ServletContextHandler, ServletHolder} import org.glassfish.jersey.server.ServerProperties import org.glassfish.jersey.servlet.ServletContainer + import org.apache.spark.SecurityManager import org.apache.spark.ui.{SparkUI, UIUtils} From 0be25254a3f045babd36011c81733acc23fe3cfc Mon Sep 17 00:00:00 2001 From: Marco Gaido Date: Wed, 27 Jun 2018 15:59:51 +0200 Subject: [PATCH 4/6] fix --- core/src/main/scala/org/apache/spark/ui/UIUtils.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/org/apache/spark/ui/UIUtils.scala b/core/src/main/scala/org/apache/spark/ui/UIUtils.scala index 4f5fd7d8d7b07..732b7528f499e 100644 --- a/core/src/main/scala/org/apache/spark/ui/UIUtils.scala +++ b/core/src/main/scala/org/apache/spark/ui/UIUtils.scala @@ -569,6 +569,6 @@ private[spark] object UIUtils extends Logging { } def buildErrorResponse(status: Response.Status, msg: String): Response = { - Response.status(Response.Status.FORBIDDEN).entity(msg).`type`(MediaType.TEXT_PLAIN).build() + Response.status(status).entity(msg).`type`(MediaType.TEXT_PLAIN).build() } } From a9baa7cc3bd894cf06f98a5b58a4c2754b11aef1 Mon Sep 17 00:00:00 2001 From: Marco Gaido Date: Wed, 27 Jun 2018 18:38:15 +0200 Subject: [PATCH 5/6] remove redundant exception --- .../scala/org/apache/spark/status/api/v1/ApiRootResource.scala | 3 --- 1 file changed, 3 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala b/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala index 256555c2a3d52..84c2ad48f1f27 100644 --- a/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala +++ b/core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala @@ -151,15 +151,12 @@ private[v1] class ForbiddenException(msg: String) extends WebApplicationExceptio UIUtils.buildErrorResponse(Response.Status.FORBIDDEN, msg)) private[v1] class NotFoundException(msg: String) extends WebApplicationException( - new NoSuchElementException(msg), UIUtils.buildErrorResponse(Response.Status.NOT_FOUND, msg)) private[v1] class ServiceUnavailable(msg: String) extends WebApplicationException( - new ServiceUnavailableException(msg), UIUtils.buildErrorResponse(Response.Status.SERVICE_UNAVAILABLE, msg)) private[v1] class BadParameterException(msg: String) extends WebApplicationException( - new IllegalArgumentException(msg), UIUtils.buildErrorResponse(Response.Status.BAD_REQUEST, msg)) { def this(param: String, exp: String, actual: String) = { this(raw"""Bad value for parameter "$param". Expected a $exp, got "$actual"""") From 0c6bbbc2d20efe4ac2a954183f91d0eb95f7b757 Mon Sep 17 00:00:00 2001 From: Marco Gaido Date: Wed, 27 Jun 2018 20:52:54 +0200 Subject: [PATCH 6/6] address comment --- .../apache/spark/status/api/v1/OneApplicationResource.scala | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala b/core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala index 1f73272c26807..32100c5704538 100644 --- a/core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala +++ b/core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala @@ -141,8 +141,7 @@ private[v1] class AbstractApplicationResource extends BaseAppResource { .build() } catch { case NonFatal(_) => - UIUtils.buildErrorResponse(Response.Status.SERVICE_UNAVAILABLE, - s"Event logs are not available for app: $appId.") + throw new ServiceUnavailable(s"Event logs are not available for app: $appId.") } }