This is an automated email from the ASF dual-hosted git repository.
feiwang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-kyuubi.git
The following commit(s) were added to refs/heads/master by this push:
new e4e88355d [KYUUBI #2634] [SUB-TASK][KPIP-4] Enhance the response error
msg
e4e88355d is described below
commit e4e88355d73e0ab0c63bd55fdf5297d925690dd3
Author: ulysses-you <[email protected]>
AuthorDate: Thu May 12 12:57:16 2022 +0800
[KYUUBI #2634] [SUB-TASK][KPIP-4] Enhance the response error msg
### _Why are the changes needed?_
closes https://github.com/apache/incubator-kyuubi/issues/2632
### _How was this patch tested?_
- [x] Add some test cases that check the changes thoroughly including
negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [ ] [Run
test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests)
locally before make a pull request
Closes #2634 from ulysses-you/error-msg.
Closes #2634
5f2bd7e5 [ulysses-you] fix
1883275b [ulysses-you] enhance rest error msg
Authored-by: ulysses-you <[email protected]>
Signed-off-by: Fei Wang <[email protected]>
---
.../engine/spark/SparkBatchProcessBuilder.scala | 11 +++--
.../kyuubi/engine/spark/SparkProcessBuilder.scala | 2 +-
.../apache/kyuubi/server/api/OpenAPIConfig.scala | 1 +
.../scala/org/apache/kyuubi/server/api/api.scala | 22 +++++++++-
.../kyuubi/server/api/v1/BatchesResource.scala | 4 +-
.../org/apache/kyuubi/server/api/v1/dto.scala | 8 ++--
.../engine/spark/SparkProcessBuilderSuite.scala | 2 +-
.../server/KyuubiRestFrontendServiceSuite.scala | 2 +-
.../server/api/v1/BatchesResourceSuite.scala | 47 ++++++++++++++++++++++
9 files changed, 86 insertions(+), 13 deletions(-)
diff --git
a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkBatchProcessBuilder.scala
b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkBatchProcessBuilder.scala
index 49313ed3e..399a5f25f 100644
---
a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkBatchProcessBuilder.scala
+++
b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkBatchProcessBuilder.scala
@@ -36,11 +36,13 @@ class SparkBatchProcessBuilder(
override def mainResource: Option[String] = Option(batchRequest.resource)
- override protected def commands: Array[String] = {
+ override protected val commands: Array[String] = {
val buffer = new ArrayBuffer[String]()
buffer += executable
- buffer += CLASS
- buffer += mainClass
+ Option(mainClass).foreach { cla =>
+ buffer += CLASS
+ buffer += cla
+ }
val batchJobTag = batchRequest.conf.get(TAG_KEY).map(_ +
",").getOrElse("") + batchId
@@ -54,7 +56,8 @@ class SparkBatchProcessBuilder(
buffer += PROXY_USER
buffer += proxyUser
- mainResource.foreach { r => buffer += r }
+ assert(mainResource.isDefined)
+ buffer += mainResource.get
batchRequest.args.foreach { arg => buffer += arg }
diff --git
a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala
b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala
index 7b9e81da6..f106d7b3c 100644
---
a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala
+++
b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala
@@ -47,7 +47,7 @@ class SparkProcessBuilder(
override def mainClass: String =
"org.apache.kyuubi.engine.spark.SparkSQLEngine"
- override protected def commands: Array[String] = {
+ override protected val commands: Array[String] = {
val buffer = new ArrayBuffer[String]()
buffer += executable
buffer += CLASS
diff --git
a/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/OpenAPIConfig.scala
b/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/OpenAPIConfig.scala
index d519ab518..c4733a0b0 100644
---
a/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/OpenAPIConfig.scala
+++
b/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/OpenAPIConfig.scala
@@ -25,4 +25,5 @@ class OpenAPIConfig extends ResourceConfig {
packages("org.apache.kyuubi.server.api.v1")
register(classOf[KyuubiOpenApiResource])
register(classOf[KyuubiScalaObjectMapper])
+ register(classOf[RestExceptionMapper])
}
diff --git
a/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/api.scala
b/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/api.scala
index ec1cade55..deadcf9ab 100644
--- a/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/api.scala
+++ b/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/api.scala
@@ -19,7 +19,9 @@ package org.apache.kyuubi.server.api
import javax.servlet.ServletContext
import javax.servlet.http.HttpServletRequest
-import javax.ws.rs.core.Context
+import javax.ws.rs.WebApplicationException
+import javax.ws.rs.core.{Context, MediaType, Response}
+import javax.ws.rs.ext.{ExceptionMapper, Provider}
import org.eclipse.jetty.server.handler.ContextHandler
@@ -36,6 +38,24 @@ private[api] trait ApiRequestContext {
final protected def fe: KyuubiRestFrontendService =
FrontendServiceContext.get(servletContext)
}
+@Provider
+class RestExceptionMapper extends ExceptionMapper[Exception] {
+ override def toResponse(exception: Exception): Response = {
+ exception match {
+ case e: WebApplicationException =>
+ Response.status(e.getResponse.getStatus)
+ .`type`(e.getResponse.getMediaType)
+ .entity(e.getMessage)
+ .build()
+ case e =>
+ Response.status(Response.Status.INTERNAL_SERVER_ERROR)
+ .`type`(MediaType.APPLICATION_JSON)
+ .entity(e.getMessage)
+ .build()
+ }
+ }
+}
+
private[api] object FrontendServiceContext {
private val attribute = getClass.getCanonicalName
diff --git
a/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala
b/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala
index 35834a977..968e8af2a 100644
---
a/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala
+++
b/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala
@@ -65,6 +65,8 @@ private[v1] class BatchesResource extends ApiRequestContext
with Logging {
@POST
@Consumes(Array(MediaType.APPLICATION_JSON))
def openBatchSession(request: BatchRequest): Batch = {
+ require(request.batchType != null, "batchType is a required parameter")
+ require(request.resource != null, "resource is a required parameter")
val userName = fe.getUserName(request.conf)
val ipAddress = AuthenticationFilter.getUserIpAddress
val sessionHandle = sessionManager.openBatchSession(
@@ -72,7 +74,7 @@ private[v1] class BatchesResource extends ApiRequestContext
with Logging {
userName,
"anonymous",
ipAddress,
- Option(request.conf).getOrElse(Map()),
+ request.conf,
request)
buildBatch(sessionHandle)
}
diff --git
a/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/dto.scala
b/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/dto.scala
index d8ef8a70c..2b1fc4bd5 100644
--- a/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/dto.scala
+++ b/kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/dto.scala
@@ -107,9 +107,9 @@ case class Field(dataType: String, value: Any)
/**
* The request body for batch job submission.
*
- * @param batchType the batch job type, such as spark, flink, etc.
+ * @param batchType the batch job type, such as spark, flink, etc. required.
* @param resource the main resource jar, required.
- * @param className the main class name, required.
+ * @param className the main class name, optional.
* @param name a name of your batch job, optional.
* @param conf arbitrary configuration properties, optional.
* @param args comma-separated list of batch job arguments, optional.
@@ -119,8 +119,8 @@ case class BatchRequest(
resource: String,
className: String,
name: String,
- conf: Map[String, String],
- args: Seq[String])
+ conf: Map[String, String] = Map.empty,
+ args: Seq[String] = Seq.empty)
case class GetBatchesResponse(
from: Int,
diff --git
a/kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala
b/kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala
index 440b667b8..802c6e7b8 100644
---
a/kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala
+++
b/kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala
@@ -266,5 +266,5 @@ class SparkProcessBuilderSuite extends KerberizedTestHelper
with MockitoSugar {
class FakeSparkProcessBuilder(config: KyuubiConf)
extends SparkProcessBuilder("fake", config) {
- override protected def commands: Array[String] = Array("ls")
+ override protected val commands: Array[String] = Array("ls")
}
diff --git
a/kyuubi-server/src/test/scala/org/apache/kyuubi/server/KyuubiRestFrontendServiceSuite.scala
b/kyuubi-server/src/test/scala/org/apache/kyuubi/server/KyuubiRestFrontendServiceSuite.scala
index 81f8c866d..94a938478 100644
---
a/kyuubi-server/src/test/scala/org/apache/kyuubi/server/KyuubiRestFrontendServiceSuite.scala
+++
b/kyuubi-server/src/test/scala/org/apache/kyuubi/server/KyuubiRestFrontendServiceSuite.scala
@@ -43,7 +43,7 @@ class KyuubiRestFrontendServiceSuite extends
RestFrontendTestHelper {
response = webTarget.path("api/v1/exception").request().get()
assert(500 == response.getStatus)
- assert(response.getStatusInfo.getReasonPhrase.equalsIgnoreCase("server
error"))
+ assert(response.getStatusInfo.getReasonPhrase.equalsIgnoreCase("Internal
Server Error"))
}
test("swagger ui") {
diff --git
a/kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/BatchesResourceSuite.scala
b/kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/BatchesResourceSuite.scala
index 7f396a2e0..e2ff28ab5 100644
---
a/kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/BatchesResourceSuite.scala
+++
b/kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/BatchesResourceSuite.scala
@@ -261,4 +261,51 @@ class BatchesResourceSuite extends KyuubiFunSuite with
RestFrontendTestHelper {
assert(getBatchListResponse6.total == 1)
sessionManager.allSessions().map(_.close())
}
+
+ test("negative request") {
+ val sparkProcessBuilder = new SparkProcessBuilder("kyuubi", conf)
+
+ // open batch session
+ Seq(
+ (
+ BatchRequest(
+ null,
+ sparkProcessBuilder.mainResource.get,
+ sparkProcessBuilder.mainClass,
+ "test-name"),
+ "batchType is a required parameter"),
+ (
+ BatchRequest(
+ "sp",
+ sparkProcessBuilder.mainResource.get,
+ sparkProcessBuilder.mainClass,
+ "test-name"),
+ "due to Batch type sp unsupported"),
+ (
+ BatchRequest(
+ "SPARK",
+ null,
+ sparkProcessBuilder.mainClass,
+ "test-name"),
+ "resource is a required parameter")).foreach { case (req, msg) =>
+ val response = webTarget.path("api/v1/batches")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .post(Entity.entity(req, MediaType.APPLICATION_JSON_TYPE))
+ assert(500 == response.getStatus)
+ assert(response.readEntity(classOf[String]).contains(msg))
+ }
+
+ // get batch by id
+ Seq(
+ ("??", "Invalid batchId: ??"),
+ (
+ "3ea7ddbe-0c35-45da-85ad-3186770181a7",
+ "Invalid batchId: 3ea7ddbe-0c35-45da-85ad-3186770181a7")).foreach {
case (batchId, msg) =>
+ val response = webTarget.path(s"api/v1/batches/$batchId")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .get
+ assert(404 == response.getStatus)
+ assert(response.readEntity(classOf[String]).contains(msg))
+ }
+ }
}
