You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "pan3793 (via GitHub)" <gi...@apache.org> on 2023/05/05 05:30:51 UTC
[GitHub] [kyuubi] pan3793 opened a new pull request, #4790: WIP: Batch V2
pan3793 opened a new pull request, #4790:
URL: https://github.com/apache/kyuubi/pull/4790
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/CONTRIBUTING.html
2. If the PR is related to an issue in https://github.com/apache/kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
## How is it done today, and what are the limits of current practice?
The current procedure of Batch Job API, called as V1
```mermaid
sequenceDiagram
participant Client
participant Server
participant Metastore
participant RM
Client ->> Server : Create Batch Job
Server ->> Server : Create Batch Operator
Server ->> Metastore : Persist Job metadata (PENDING)
Server ->> Server : Put Batch Operator into Execution Pool
Server ->> Client : Batch Job Info
Server -x RM : Submit Applicition (async in Execution Pool)
loop Application Check
Server ->> RM : Query Application Status
Server ->> Metastore : Update Batch Status
end
Client ->> Server : Query Batch Job Info
alt KyuubiInstance matched
Server ->> Client : Batch Job Info
else
Server ->> Server : Forward Request to expected KyuubiInstance
end
Client ->> Server : Fetch Batch Job logs
alt KyuubiInstance matched
Server ->> Client : Batch Job logs
else
Server ->> Server : Forward Request to expected KyuubiInstance
end
Client ->> Server : Close Batch Job
alt KyuubiInstance matched
Server ->> RM : Close the Application
Server ->> Metastore : Update Batch Status
Server ->> Client : Closed Batch Job Info
else
Server ->> Server : Forward Request to expected KyuubiInstance
end
```
V2
```mermaid
sequenceDiagram
participant Client
participant Server
participant Metastore
participant RM
Client ->> Server : Create Batch Job
Server ->> Metastore : Persist Job metadata (INITIALIZED)
Server ->> Client : Batch Job Info
loop Forever
Server ->> Metastore : Pick up and lock INITIALIZED job
Server ->> RM : Submit Application
Server ->> RM : Query Application Status
Server ->> Metastore : Update Batch Status
end
Client ->> Server : Query Batch Job Info
Server ->> Metastore : Query Batch Job Info
Server ->> Client : Batch Job Info
Client ->> Server : Fetch Batch Job logs
alt KyuubiInstance matched
Server ->> Client : Batch Job logs
else
Server ->> Server : Forward Request to expected KyuubiInstance
end
Client ->> Server : Close Batch Job
alt KyuubiInstance matched
Server ->> RM : Close the Application
Server ->> Metastore : Update Batch Status
Server ->> Client : Closed Batch Job Info
else
Server ->> Server : Forward Request to expected KyuubiInstance
end
```
### _How was this patch tested?_
- [ ] 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.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on pull request #4790: Initial implement Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#issuecomment-1658083604
The last change only renames a variable, thanks, merging to master
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1278841611
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala:
##########
@@ -58,6 +58,10 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
private lazy val internalConnectTimeout =
fe.getConf.get(BATCH_INTERNAL_REST_CLIENT_CONNECT_TIMEOUT).toInt
+ private def batchV2Enabled(reqConf: Map[String, String]): Boolean = {
+ reqConf.getOrElse(BATCH_API_VERSION.key, fe.getConf.get(BATCH_API_VERSION)) == "2"
Review Comment:
I suppose `kyuubi.batch.submitter.enabled` will be removed in the future, and always be enabled when the REST frontend is enabled.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1272894945
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala:
##########
@@ -58,6 +58,10 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
private lazy val internalConnectTimeout =
fe.getConf.get(BATCH_INTERNAL_REST_CLIENT_CONNECT_TIMEOUT).toInt
+ private def batchV2Enabled(reqConf: Map[String, String]): Boolean = {
+ reqConf.getOrElse(BATCH_API_VERSION.key, fe.getConf.get(BATCH_API_VERSION)) == "2"
Review Comment:
so the client can enable batch v2?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] bowenliang123 commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1279079343
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/BatchesResourceSuite.scala:
##########
@@ -79,9 +105,18 @@ class BatchesResourceSuite extends KyuubiFunSuite with RestFrontendTestHelper wi
val response = webTarget.path("api/v1/batches")
.request(MediaType.APPLICATION_JSON_TYPE)
.post(Entity.entity(requestObj, MediaType.APPLICATION_JSON_TYPE))
- assert(200 == response.getStatus)
+ assert(response.getStatus === 200)
Review Comment:
minor: I would prefer `assertResult` rather than `assert`. more clear in expected results.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1278841611
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala:
##########
@@ -58,6 +58,10 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
private lazy val internalConnectTimeout =
fe.getConf.get(BATCH_INTERNAL_REST_CLIENT_CONNECT_TIMEOUT).toInt
+ private def batchV2Enabled(reqConf: Map[String, String]): Boolean = {
+ reqConf.getOrElse(BATCH_API_VERSION.key, fe.getConf.get(BATCH_API_VERSION)) == "2"
Review Comment:
> so, in server side, the batch v2 is always supported?
No, now. But should be right in the future.
I suppose `kyuubi.batch.submitter.enabled` will be removed in the future, and always be enabled when the REST frontend is enabled.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 closed pull request #4790: Initial implement Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 closed pull request #4790: Initial implement Batch V2
URL: https://github.com/apache/kyuubi/pull/4790
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1278862946
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala:
##########
@@ -58,6 +58,10 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
private lazy val internalConnectTimeout =
fe.getConf.get(BATCH_INTERNAL_REST_CLIENT_CONNECT_TIMEOUT).toInt
+ private def batchV2Enabled(reqConf: Map[String, String]): Boolean = {
+ reqConf.getOrElse(BATCH_API_VERSION.key, fe.getConf.get(BATCH_API_VERSION)) == "2"
Review Comment:
yea, it's a valid concern, I planned to use `/api/v2` at the beginning, but during the implementation, I found there is nothing change in the user API, so I choose to introduce a parameter instead of bumping the API version.
Any suggestion for the name here? `kyuubi.batch.impl=1,2`?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] zhaohehuhu commented on pull request #4790: WIP: Batch V2
Posted by "zhaohehuhu (via GitHub)" <gi...@apache.org>.
zhaohehuhu commented on PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#issuecomment-1541563182
Kyuubi UI can benefit a lot from this feature, since Job info already persist in MetaStore and these info are shared across all the server instances.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1273137874
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala:
##########
@@ -58,6 +58,10 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
private lazy val internalConnectTimeout =
fe.getConf.get(BATCH_INTERNAL_REST_CLIENT_CONNECT_TIMEOUT).toInt
+ private def batchV2Enabled(reqConf: Map[String, String]): Boolean = {
+ reqConf.getOrElse(BATCH_API_VERSION.key, fe.getConf.get(BATCH_API_VERSION)) == "2"
Review Comment:
Yes, it's allowed to explicitly enable batch v2 at the app level on the client side.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1272895127
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala:
##########
@@ -58,6 +58,10 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
private lazy val internalConnectTimeout =
fe.getConf.get(BATCH_INTERNAL_REST_CLIENT_CONNECT_TIMEOUT).toInt
+ private def batchV2Enabled(reqConf: Map[String, String]): Boolean = {
+ reqConf.getOrElse(BATCH_API_VERSION.key, fe.getConf.get(BATCH_API_VERSION)) == "2"
Review Comment:
is it a server side behavior?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1278851779
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala:
##########
@@ -58,6 +58,10 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
private lazy val internalConnectTimeout =
fe.getConf.get(BATCH_INTERNAL_REST_CLIENT_CONNECT_TIMEOUT).toInt
+ private def batchV2Enabled(reqConf: Map[String, String]): Boolean = {
+ reqConf.getOrElse(BATCH_API_VERSION.key, fe.getConf.get(BATCH_API_VERSION)) == "2"
Review Comment:
When the server sets `kyuubi.batch.submitter.enabled=false`, and the client creates a batch with `kyuubi.batch.api.version=2`, the server would return an error.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1278855771
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala:
##########
@@ -58,6 +58,10 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
private lazy val internalConnectTimeout =
fe.getConf.get(BATCH_INTERNAL_REST_CLIENT_CONNECT_TIMEOUT).toInt
+ private def batchV2Enabled(reqConf: Map[String, String]): Boolean = {
+ reqConf.getOrElse(BATCH_API_VERSION.key, fe.getConf.get(BATCH_API_VERSION)) == "2"
Review Comment:
I think this config item is confused. If I am a customer, I may think that the api version here is rest api version /api/v1 or /api/v2.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on pull request #4790: Initial implement Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#issuecomment-1645518909
cc @turboFei, this is the initial implementation of batch v2, after some paving patches, this PR becomes clean and small enough to review. BTW, switches are added to ensure no change with existing behaviors.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on pull request #4790: Initial implement Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#issuecomment-1657693624
All comments are addressed, I'm going to merge this one after CI green if no further comments.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1278837636
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala:
##########
@@ -58,6 +58,10 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
private lazy val internalConnectTimeout =
fe.getConf.get(BATCH_INTERNAL_REST_CLIENT_CONNECT_TIMEOUT).toInt
+ private def batchV2Enabled(reqConf: Map[String, String]): Boolean = {
+ reqConf.getOrElse(BATCH_API_VERSION.key, fe.getConf.get(BATCH_API_VERSION)) == "2"
Review Comment:
so, in server side, the batch v2 is always supported?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1279071733
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1612,6 +1612,41 @@ object KyuubiConf {
.booleanConf
.createWithDefault(true)
+ val BATCH_API_VERSION: ConfigEntry[String] =
Review Comment:
please modify the variable name as well.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1278848928
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala:
##########
@@ -58,6 +58,10 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
private lazy val internalConnectTimeout =
fe.getConf.get(BATCH_INTERNAL_REST_CLIENT_CONNECT_TIMEOUT).toInt
+ private def batchV2Enabled(reqConf: Map[String, String]): Boolean = {
+ reqConf.getOrElse(BATCH_API_VERSION.key, fe.getConf.get(BATCH_API_VERSION)) == "2"
Review Comment:
what would happen if client enable batch v2 but it is not supported in server side?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1278885204
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala:
##########
@@ -58,6 +58,10 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
private lazy val internalConnectTimeout =
fe.getConf.get(BATCH_INTERNAL_REST_CLIENT_CONNECT_TIMEOUT).toInt
+ private def batchV2Enabled(reqConf: Map[String, String]): Boolean = {
+ reqConf.getOrElse(BATCH_API_VERSION.key, fe.getConf.get(BATCH_API_VERSION)) == "2"
Review Comment:
Prefer: kyuubi.batch.impl.version=1,2
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] bowenliang123 commented on pull request #4790: WIP: Batch V2
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#issuecomment-1536350993
+1 for this feature.
Try to summarize the key changes in V2 batch support,
1. decoupling job lifecycle from RM by early/eagerly delegating job Metadata to Metasotre
2. leave newly created batch with INITIALIZED status, and return immediately avoiding waiting for pending submission
3. server responsible for whole job lifecyle and update status to Metastore continuously
4. fetch job info from MetaStore instead of forwarding requests to engine instance
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] zwangsheng commented on pull request #4790: WIP: Batch V2
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#issuecomment-1537025237
+ 1 for this.
It sounds tempting for Kyuubi Server to pick tasks to run under its own load.
In addition, it's better to uniformly obtain the batch status from metahub, which reduces several requests than Kyuubi Servers mutual forwarding inquiries.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1273140883
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiBatchService.scala:
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.server
+
+import java.util.concurrent.atomic.AtomicBoolean
+
+import org.apache.kyuubi.config.KyuubiConf.BATCH_SUBMITTER_THREADS
+import org.apache.kyuubi.operation.OperationState
+import org.apache.kyuubi.server.metadata.MetadataManager
+import org.apache.kyuubi.server.metadata.api.Metadata
+import org.apache.kyuubi.service.{AbstractService, Serverable}
+import org.apache.kyuubi.session.KyuubiSessionManager
+import org.apache.kyuubi.util.ThreadUtils
+
+class KyuubiBatchService(
+ server: Serverable,
+ sessionManager: KyuubiSessionManager)
+ extends AbstractService(classOf[KyuubiBatchService].getSimpleName) {
+
+ private lazy val restFrontend = server.frontendServices
+ .filter(_.isInstanceOf[KyuubiRestFrontendService])
+ .head
+
+ private def kyuubiInstance: String = restFrontend.connectionUrl
+
+ // TODO expose metrics, including pending/running/succeeded/failed batches
+ // TODO handle dangling batches, e.g. batch is picked and changed state to pending,
+ // but the Server crashed before submitting or updating status to metastore
+
+ private lazy val metadataManager: MetadataManager = sessionManager.metadataManager.get
+ private val running: AtomicBoolean = new AtomicBoolean(false)
+ private lazy val batchExecutor = ThreadUtils
+ .newDaemonFixedThreadPool(conf.get(BATCH_SUBMITTER_THREADS), "kyuubi-batch-submitter")
+
+ def cancelUnscheduledBatch(batchId: String): Boolean = {
+ metadataManager.cancelUnscheduledBatch(batchId)
+ }
+
+ def countBatch(
+ batchType: String,
+ batchUser: Option[String],
+ batchState: Option[String] = None,
+ kyuubiInstance: Option[String] = None): Int = {
+ metadataManager.countBatch(
+ batchType,
+ batchUser.orNull,
+ batchState.orNull,
+ kyuubiInstance.orNull)
+ }
+
+ override def start(): Unit = {
+ assert(running.compareAndSet(false, true))
+ val submitTask: Runnable = () => {
+ while (running.get) {
+ metadataManager.pickBatchForSubmitting(kyuubiInstance) match {
+ case None => Thread.sleep(1000)
+ case Some(metadata) =>
+ val batchId = metadata.identifier
+ info(s"$batchId is picked for submission.")
+ val batchSession = sessionManager.createBatchSession(
+ metadata.username,
+ "anonymous",
+ metadata.ipAddress,
+ metadata.requestConf,
+ metadata.engineType,
+ Option(metadata.requestName),
+ metadata.resource,
+ metadata.className,
+ metadata.requestConf,
+ metadata.requestArgs,
+ Some(metadata), // TODO some logic need to fix since it's not from recovery
+ shouldRunAsync = true)
+ val metadataForUpdate = Metadata(
+ identifier = batchId,
+ kyuubiInstance = kyuubiInstance,
+ requestConf = batchSession.optimizedConf,
+ clusterManager = batchSession.batchJobSubmissionOp.builder.clusterManager())
+ metadataManager.updateMetadata(metadataForUpdate, asyncRetryOnError = false)
+ val sessionHandle = sessionManager.openBatchSession(batchSession)
+ var terminated = false
+ while (!terminated) { // block until batch job finished
+ terminated = sessionManager.getBatchSession(sessionHandle).map { batchSession =>
+ val batchOp = batchSession.batchJobSubmissionOp
+ OperationState.isTerminal(batchOp.getStatus.state)
+ }.getOrElse {
+ error(s"Batch Session $batchId is not existed, marked as finished")
+ true
+ }
+ // should we always treat metastore as the single of truth?
+ //
+ // terminated = metadataManager.getBatchSessionMetadata(batchId) match {
+ // case Some(metadata) =>
+ // OperationState.isTerminal(OperationState.withName(metadata.state))
+ // case None =>
+ // error(s"$batchId is not existed in metastore, assume it is finished")
+ // true
+ // }
+ if (!terminated) Thread.sleep(1000)
+ }
+ info(s"$batchId is finished.")
+ }
+ }
+ }
+ (0 until batchExecutor.getCorePoolSize).foreach(_ => batchExecutor.submit(submitTask))
Review Comment:
As the offline discussion, set `kyuubi.batch.submitter.threads` default value to 100 same as `kyuubi.backend.server.exec.pool.size` to achieve same performance with existing v1
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1272899767
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiBatchService.scala:
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.server
+
+import java.util.concurrent.atomic.AtomicBoolean
+
+import org.apache.kyuubi.config.KyuubiConf.BATCH_SUBMITTER_THREADS
+import org.apache.kyuubi.operation.OperationState
+import org.apache.kyuubi.server.metadata.MetadataManager
+import org.apache.kyuubi.server.metadata.api.Metadata
+import org.apache.kyuubi.service.{AbstractService, Serverable}
+import org.apache.kyuubi.session.KyuubiSessionManager
+import org.apache.kyuubi.util.ThreadUtils
+
+class KyuubiBatchService(
+ server: Serverable,
+ sessionManager: KyuubiSessionManager)
+ extends AbstractService(classOf[KyuubiBatchService].getSimpleName) {
+
+ private lazy val restFrontend = server.frontendServices
+ .filter(_.isInstanceOf[KyuubiRestFrontendService])
+ .head
+
+ private def kyuubiInstance: String = restFrontend.connectionUrl
+
+ // TODO expose metrics, including pending/running/succeeded/failed batches
+ // TODO handle dangling batches, e.g. batch is picked and changed state to pending,
+ // but the Server crashed before submitting or updating status to metastore
+
+ private lazy val metadataManager: MetadataManager = sessionManager.metadataManager.get
+ private val running: AtomicBoolean = new AtomicBoolean(false)
+ private lazy val batchExecutor = ThreadUtils
+ .newDaemonFixedThreadPool(conf.get(BATCH_SUBMITTER_THREADS), "kyuubi-batch-submitter")
+
+ def cancelUnscheduledBatch(batchId: String): Boolean = {
+ metadataManager.cancelUnscheduledBatch(batchId)
+ }
+
+ def countBatch(
+ batchType: String,
+ batchUser: Option[String],
+ batchState: Option[String] = None,
+ kyuubiInstance: Option[String] = None): Int = {
+ metadataManager.countBatch(
+ batchType,
+ batchUser.orNull,
+ batchState.orNull,
+ kyuubiInstance.orNull)
+ }
+
+ override def start(): Unit = {
+ assert(running.compareAndSet(false, true))
+ val submitTask: Runnable = () => {
+ while (running.get) {
+ metadataManager.pickBatchForSubmitting(kyuubiInstance) match {
+ case None => Thread.sleep(1000)
+ case Some(metadata) =>
+ val batchId = metadata.identifier
+ info(s"$batchId is picked for submission.")
+ val batchSession = sessionManager.createBatchSession(
+ metadata.username,
+ "anonymous",
+ metadata.ipAddress,
+ metadata.requestConf,
+ metadata.engineType,
+ Option(metadata.requestName),
+ metadata.resource,
+ metadata.className,
+ metadata.requestConf,
+ metadata.requestArgs,
+ Some(metadata), // TODO some logic need to fix since it's not from recovery
+ shouldRunAsync = true)
+ val metadataForUpdate = Metadata(
+ identifier = batchId,
+ kyuubiInstance = kyuubiInstance,
+ requestConf = batchSession.optimizedConf,
+ clusterManager = batchSession.batchJobSubmissionOp.builder.clusterManager())
+ metadataManager.updateMetadata(metadataForUpdate, asyncRetryOnError = false)
+ val sessionHandle = sessionManager.openBatchSession(batchSession)
+ var terminated = false
+ while (!terminated) { // block until batch job finished
+ terminated = sessionManager.getBatchSession(sessionHandle).map { batchSession =>
+ val batchOp = batchSession.batchJobSubmissionOp
+ OperationState.isTerminal(batchOp.getStatus.state)
+ }.getOrElse {
+ error(s"Batch Session $batchId is not existed, marked as finished")
+ true
+ }
+ // should we always treat metastore as the single of truth?
+ //
+ // terminated = metadataManager.getBatchSessionMetadata(batchId) match {
+ // case Some(metadata) =>
+ // OperationState.isTerminal(OperationState.withName(metadata.state))
+ // case None =>
+ // error(s"$batchId is not existed in metastore, assume it is finished")
+ // true
+ // }
+ if (!terminated) Thread.sleep(1000)
+ }
+ info(s"$batchId is finished.")
+ }
+ }
+ }
+ (0 until batchExecutor.getCorePoolSize).foreach(_ => batchExecutor.submit(submitTask))
Review Comment:
does it mean that, for batch v2, the max concurrent running batch size = batchExecutor.getCorePoolSize.
It seems is a big performance regression.
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiBatchService.scala:
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.server
+
+import java.util.concurrent.atomic.AtomicBoolean
+
+import org.apache.kyuubi.config.KyuubiConf.BATCH_SUBMITTER_THREADS
+import org.apache.kyuubi.operation.OperationState
+import org.apache.kyuubi.server.metadata.MetadataManager
+import org.apache.kyuubi.server.metadata.api.Metadata
+import org.apache.kyuubi.service.{AbstractService, Serverable}
+import org.apache.kyuubi.session.KyuubiSessionManager
+import org.apache.kyuubi.util.ThreadUtils
+
+class KyuubiBatchService(
+ server: Serverable,
+ sessionManager: KyuubiSessionManager)
+ extends AbstractService(classOf[KyuubiBatchService].getSimpleName) {
+
+ private lazy val restFrontend = server.frontendServices
+ .filter(_.isInstanceOf[KyuubiRestFrontendService])
+ .head
+
+ private def kyuubiInstance: String = restFrontend.connectionUrl
+
+ // TODO expose metrics, including pending/running/succeeded/failed batches
+ // TODO handle dangling batches, e.g. batch is picked and changed state to pending,
+ // but the Server crashed before submitting or updating status to metastore
+
+ private lazy val metadataManager: MetadataManager = sessionManager.metadataManager.get
+ private val running: AtomicBoolean = new AtomicBoolean(false)
+ private lazy val batchExecutor = ThreadUtils
+ .newDaemonFixedThreadPool(conf.get(BATCH_SUBMITTER_THREADS), "kyuubi-batch-submitter")
+
+ def cancelUnscheduledBatch(batchId: String): Boolean = {
+ metadataManager.cancelUnscheduledBatch(batchId)
+ }
+
+ def countBatch(
+ batchType: String,
+ batchUser: Option[String],
+ batchState: Option[String] = None,
+ kyuubiInstance: Option[String] = None): Int = {
+ metadataManager.countBatch(
+ batchType,
+ batchUser.orNull,
+ batchState.orNull,
+ kyuubiInstance.orNull)
+ }
+
+ override def start(): Unit = {
+ assert(running.compareAndSet(false, true))
+ val submitTask: Runnable = () => {
+ while (running.get) {
+ metadataManager.pickBatchForSubmitting(kyuubiInstance) match {
+ case None => Thread.sleep(1000)
+ case Some(metadata) =>
+ val batchId = metadata.identifier
+ info(s"$batchId is picked for submission.")
+ val batchSession = sessionManager.createBatchSession(
+ metadata.username,
+ "anonymous",
+ metadata.ipAddress,
+ metadata.requestConf,
+ metadata.engineType,
+ Option(metadata.requestName),
+ metadata.resource,
+ metadata.className,
+ metadata.requestConf,
+ metadata.requestArgs,
+ Some(metadata), // TODO some logic need to fix since it's not from recovery
+ shouldRunAsync = true)
+ val metadataForUpdate = Metadata(
+ identifier = batchId,
+ kyuubiInstance = kyuubiInstance,
+ requestConf = batchSession.optimizedConf,
+ clusterManager = batchSession.batchJobSubmissionOp.builder.clusterManager())
+ metadataManager.updateMetadata(metadataForUpdate, asyncRetryOnError = false)
+ val sessionHandle = sessionManager.openBatchSession(batchSession)
+ var terminated = false
+ while (!terminated) { // block until batch job finished
+ terminated = sessionManager.getBatchSession(sessionHandle).map { batchSession =>
+ val batchOp = batchSession.batchJobSubmissionOp
+ OperationState.isTerminal(batchOp.getStatus.state)
+ }.getOrElse {
+ error(s"Batch Session $batchId is not existed, marked as finished")
+ true
+ }
+ // should we always treat metastore as the single of truth?
+ //
+ // terminated = metadataManager.getBatchSessionMetadata(batchId) match {
+ // case Some(metadata) =>
+ // OperationState.isTerminal(OperationState.withName(metadata.state))
+ // case None =>
+ // error(s"$batchId is not existed in metastore, assume it is finished")
+ // true
+ // }
+ if (!terminated) Thread.sleep(1000)
+ }
+ info(s"$batchId is finished.")
+ }
+ }
+ }
+ (0 until batchExecutor.getCorePoolSize).foreach(_ => batchExecutor.submit(submitTask))
Review Comment:
does it mean that, for batch v2, the max concurrent running batch size = batchExecutor.getCorePoolSize?
It seems is a big performance regression.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1278862946
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala:
##########
@@ -58,6 +58,10 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
private lazy val internalConnectTimeout =
fe.getConf.get(BATCH_INTERNAL_REST_CLIENT_CONNECT_TIMEOUT).toInt
+ private def batchV2Enabled(reqConf: Map[String, String]): Boolean = {
+ reqConf.getOrElse(BATCH_API_VERSION.key, fe.getConf.get(BATCH_API_VERSION)) == "2"
Review Comment:
yea, it's a valid concern, I planned to use `/api/v2` at the beginning, but during the implementation, I found there is nothing change in the user API, so I choose to introduce a parameter instead of bumping the API version.
Any suggestion for the name here? `kyuubi.batch.implementation=1,2`?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] zhaohehuhu commented on pull request #4790: WIP: Batch V2
Posted by "zhaohehuhu (via GitHub)" <gi...@apache.org>.
zhaohehuhu commented on PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#issuecomment-1541569311
> 3\. xpose global metrics of batch jobs when using sharable storage as metastore backend, e.g. we can easily get how many batches are queued in metastore, and how many batches are managed by each Kyuubi server, by querying the metastore backend directly or metrics exposed by each Kyuubi server.
awesome !!!
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on pull request #4790: WIP: Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#issuecomment-1541566862
> Kyuubi UI can benefit a lot from this feature, since Job info already persist in MetaStore and these info are shared across all the server instances.
Thanks for being interested in this design, you are right, this is just what I mentioned in Pros 3
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei commented on pull request #4790: WIP: Batch V2
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#issuecomment-1602627827
```
BatchesV2ResourceSuite:
- open batch session *** FAILED ***
The code passed to eventually never returned normally. Attempted 20 times over 10.068249206 seconds. Last failure message: logs.exists(((x$2: String) => x$2.contains("/bin/spark-submit"))) was false. (BatchesResourceSuite.scala:165)
```
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] turboFei commented on pull request #4790: WIP: Batch V2
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#issuecomment-1536974820
> +1 for this feature.
>
> Try to summarize the key changes in V2 batch support,
>
> 1. decoupling job lifecycle from RM by early/eagerly delegating job Metadata to Metasotre
>
> 2. leave newly created batch with INITIALIZED status, and return immediately avoiding waiting for pending submission
>
> 3. server responsible for whole job lifecyle and actively monitor and update job info/status to Metastore continuously
>
> 4. fetch job info from MetaStore instead of forwarding requests to engine instance
I think 1 and 2 are incorrect.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on pull request #4790: WIP: Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#issuecomment-1535718940
cc @turboFei, this is a PoC of batch API v2 we discussed before, it's still work-in-progress, I will do some refactoring in independent PRs to reduce the following PR size
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] codecov-commenter commented on pull request #4790: WIP: Batch V2
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#issuecomment-1537113472
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/4790?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#4790](https://app.codecov.io/gh/apache/kyuubi/pull/4790?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f51a4a9) into [master](https://app.codecov.io/gh/apache/kyuubi/commit/3108c8e1a599d07e9637fb31cd09eba0d6bb0d56?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3108c8e) will **decrease** coverage by `0.14%`.
> The diff coverage is `45.89%`.
```diff
@@ Coverage Diff @@
## master #4790 +/- ##
============================================
- Coverage 57.96% 57.82% -0.14%
Complexity 13 13
============================================
Files 581 582 +1
Lines 32458 32595 +137
Branches 4312 4323 +11
============================================
+ Hits 18813 18849 +36
- Misses 11832 11929 +97
- Partials 1813 1817 +4
```
| [Impacted Files](https://app.codecov.io/gh/apache/kyuubi/pull/4790?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...pache/kyuubi/server/KyuubiBatchSubmitService.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4790?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpQmF0Y2hTdWJtaXRTZXJ2aWNlLnNjYWxh) | `0.00% <0.00%> (ø)` | |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4790?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `51.04% <0.00%> (-2.81%)` | :arrow_down: |
| [...pache/kyuubi/server/metadata/MetadataManager.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4790?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvbWV0YWRhdGEvTWV0YWRhdGFNYW5hZ2VyLnNjYWxh) | `80.23% <0.00%> (-0.49%)` | :arrow_down: |
| [...yuubi/server/metadata/jdbc/JDBCMetadataStore.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4790?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvbWV0YWRhdGEvamRiYy9KREJDTWV0YWRhdGFTdG9yZS5zY2FsYQ==) | `82.46% <0.00%> (-4.56%)` | :arrow_down: |
| [.../apache/kyuubi/server/api/v1/BatchesResource.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4790?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvYXBpL3YxL0JhdGNoZXNSZXNvdXJjZS5zY2FsYQ==) | `66.79% <33.33%> (-3.87%)` | :arrow_down: |
| [...g/apache/kyuubi/operation/BatchJobSubmission.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4790?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vQmF0Y2hKb2JTdWJtaXNzaW9uLnNjYWxh) | `72.34% <55.55%> (-2.00%)` | :arrow_down: |
| [...g/apache/kyuubi/session/KyuubiSessionManager.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4790?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0t5dXViaVNlc3Npb25NYW5hZ2VyLnNjYWxh) | `84.18% <61.53%> (-10.07%)` | :arrow_down: |
| [...apache/kyuubi/session/KyuubiBatchSessionImpl.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4790?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0t5dXViaUJhdGNoU2Vzc2lvbkltcGwuc2NhbGE=) | `84.46% <92.59%> (+0.30%)` | :arrow_up: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4790?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `97.18% <100.00%> (+0.02%)` | :arrow_up: |
| [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4790?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `82.89% <100.00%> (+0.22%)` | :arrow_up: |
... and [6 files with indirect coverage changes](https://app.codecov.io/gh/apache/kyuubi/pull/4790/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1278886440
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/BatchesResource.scala:
##########
@@ -58,6 +58,10 @@ private[v1] class BatchesResource extends ApiRequestContext with Logging {
private lazy val internalConnectTimeout =
fe.getConf.get(BATCH_INTERNAL_REST_CLIENT_CONNECT_TIMEOUT).toInt
+ private def batchV2Enabled(reqConf: Map[String, String]): Boolean = {
+ reqConf.getOrElse(BATCH_API_VERSION.key, fe.getConf.get(BATCH_API_VERSION)) == "2"
Review Comment:
sgtm, will change soon.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1279091430
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/BatchesResourceSuite.scala:
##########
@@ -79,9 +105,18 @@ class BatchesResourceSuite extends KyuubiFunSuite with RestFrontendTestHelper wi
val response = webTarget.path("api/v1/batches")
.request(MediaType.APPLICATION_JSON_TYPE)
.post(Entity.entity(requestObj, MediaType.APPLICATION_JSON_TYPE))
- assert(200 == response.getStatus)
+ assert(response.getStatus === 200)
Review Comment:
I know your point, but I suppose `assert(result === expected)` is generic too, it is used widely in scalatest.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] bowenliang123 commented on a diff in pull request #4790: Initial implement Batch V2
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #4790:
URL: https://github.com/apache/kyuubi/pull/4790#discussion_r1279099711
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/BatchesResourceSuite.scala:
##########
@@ -79,9 +105,18 @@ class BatchesResourceSuite extends KyuubiFunSuite with RestFrontendTestHelper wi
val response = webTarget.path("api/v1/batches")
.request(MediaType.APPLICATION_JSON_TYPE)
.post(Entity.entity(requestObj, MediaType.APPLICATION_JSON_TYPE))
- assert(200 == response.getStatus)
+ assert(response.getStatus === 200)
Review Comment:
Yes, you're right. Although putting the object in the first place raises NPE concerns. LGTM.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org