[SPARK-24660][SHS] Show correct error pages when downloading logs #21644

mgaido91 · 2018-06-26T14:15:38Z

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:

Unauthorized user
Non-existing application

After the patch

Unauthorized user
Non-existing application

mgaido91 · 2018-06-26T14:16:05Z

cc @attilapiros @jerryshao @vanzin

attilapiros · 2018-06-26T15:13:03Z

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())


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 serverErrormethod just sets the status to Status.INTERNAL_SERVER_ERROR which overwritten by right away with Response.Status.SERVICE_UNAVAILABLE.

SparkQA · 2018-06-26T18:24:30Z

Test build #92343 has finished for PR 21644 at commit 8b26a53.

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

mgaido91 · 2018-06-27T09:50:39Z

thanks for you comment @attilapiros. I followed it. Any more comments on this?

SparkQA · 2018-06-27T09:54:59Z

Test build #92378 has finished for PR 21644 at commit 9934d47.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

jerryshao · 2018-06-27T13:03:46Z

@mgaido91 , would you please check all other response to see if it returns as expected, not only download link.

SparkQA · 2018-06-27T13:49:21Z

Test build #92380 has finished for PR 21644 at commit 43efe0f.

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

mgaido91 · 2018-06-27T15:18:53Z

@jerryshao yes, I checked other too, here it is some examples:

attilapiros

No other issue I can see.
LGTM

mgaido91 · 2018-06-27T15:24:23Z

thanks @attilapiros for taking a look at this!

vanzin · 2018-06-27T15:52:09Z

core/src/main/scala/org/apache/spark/status/api/v1/ApiRootResource.scala

-    .entity(ErrorWrapper(msg))
-    .build()
-)
+    new ServiceUnavailableException(msg),


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.

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.

Hmm, if that's the case then probably the wrappers can save some code (since they abstract the building of the response)...

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?

PS in any case, I'd save the BadParameterException as it does save a lot of duplicated code with the constructor defined there.

I think it's ok to leave (almost) as is. It avoids 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.

I agree, I updated with your suggestion, thanks

vanzin · 2018-06-27T15:52:51Z

core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala

-          .status(Response.Status.SERVICE_UNAVAILABLE)
-          .build()
+      case NonFatal(_) =>
+        UIUtils.buildErrorResponse(Response.Status.SERVICE_UNAVAILABLE,


If doing the cleanup, it's probably ok to just throw the exception here.

This could just throw ServiceUnavailable, no?

no, we need to set the type, otherwise it returns the response as octet-stream which is causing the issue..

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.

oh, now I see what you mean, sorry, I didn't get it. I am doing that, thanks.

vanzin

LGTM pending tests.

vanzin · 2018-06-27T17:18:44Z

core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala

-          .status(Response.Status.SERVICE_UNAVAILABLE)
-          .build()
+      case NonFatal(_) =>
+        UIUtils.buildErrorResponse(Response.Status.SERVICE_UNAVAILABLE,


This could just throw ServiceUnavailable, no?

SparkQA · 2018-06-27T18:24:42Z

Test build #92384 has finished for PR 21644 at commit 0be2525.

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

SparkQA · 2018-06-27T21:07:04Z

Test build #92386 has finished for PR 21644 at commit a9baa7c.

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

vanzin · 2018-06-27T21:25:52Z

Merging to master.

SparkQA · 2018-06-27T23:21:38Z

Test build #92392 has finished for PR 21644 at commit 0c6bbbc.

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

[SPARK-24660][SHS] Show correct error pages when downloading logs

8b26a53

attilapiros reviewed Jun 26, 2018

View reviewed changes

refactor adding one method to UIUtils

9934d47

fix scalastyle

43efe0f

fix

0be2525

attilapiros reviewed Jun 27, 2018

View reviewed changes

attilapiros approved these changes Jun 27, 2018

View reviewed changes

vanzin reviewed Jun 27, 2018

View reviewed changes

remove redundant exception

a9baa7c

vanzin reviewed Jun 27, 2018

View reviewed changes

address comment

0c6bbbc

asfgit closed this in 776befb Jun 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-24660][SHS] Show correct error pages when downloading logs #21644

[SPARK-24660][SHS] Show correct error pages when downloading logs #21644

mgaido91 commented Jun 26, 2018

mgaido91 commented Jun 26, 2018

attilapiros Jun 26, 2018

SparkQA commented Jun 26, 2018

mgaido91 commented Jun 27, 2018

SparkQA commented Jun 27, 2018

jerryshao commented Jun 27, 2018

SparkQA commented Jun 27, 2018

mgaido91 commented Jun 27, 2018 •

edited

Loading

attilapiros left a comment

mgaido91 commented Jun 27, 2018

vanzin Jun 27, 2018

mgaido91 Jun 27, 2018

vanzin Jun 27, 2018

mgaido91 Jun 27, 2018 •

edited

Loading

vanzin Jun 27, 2018 •

edited

Loading

mgaido91 Jun 27, 2018

vanzin Jun 27, 2018

vanzin Jun 27, 2018

mgaido91 Jun 27, 2018

vanzin Jun 27, 2018

mgaido91 Jun 27, 2018

vanzin left a comment

vanzin Jun 27, 2018

SparkQA commented Jun 27, 2018

SparkQA commented Jun 27, 2018

vanzin commented Jun 27, 2018

SparkQA commented Jun 27, 2018

[SPARK-24660][SHS] Show correct error pages when downloading logs #21644

[SPARK-24660][SHS] Show correct error pages when downloading logs #21644

Conversation

mgaido91 commented Jun 26, 2018

What changes were proposed in this pull request?

How was this patch tested?

mgaido91 commented Jun 26, 2018

Choose a reason for hiding this comment

SparkQA commented Jun 26, 2018

mgaido91 commented Jun 27, 2018

SparkQA commented Jun 27, 2018

jerryshao commented Jun 27, 2018

SparkQA commented Jun 27, 2018

mgaido91 commented Jun 27, 2018 • edited Loading

attilapiros left a comment

Choose a reason for hiding this comment

mgaido91 commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgaido91 Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

vanzin Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanzin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 27, 2018

SparkQA commented Jun 27, 2018

vanzin commented Jun 27, 2018

SparkQA commented Jun 27, 2018

mgaido91 commented Jun 27, 2018 •

edited

Loading

mgaido91 Jun 27, 2018 •

edited

Loading

vanzin Jun 27, 2018 •

edited

Loading