You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/04/06 10:27:00 UTC

[GitHub] [incubator-kyuubi] jiaoqingbo opened a new pull request, #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

jiaoqingbo opened a new pull request, #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286

   <!--
   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/contributions.html
     2. If the PR is related to an issue in https://github.com/apache/incubator-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.
   -->
   fix #2281 
   
   ### _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.apache.org/docs/latest/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] [incubator-kyuubi] zhouyifan279 commented on pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
zhouyifan279 commented on PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#issuecomment-1095819837

   @jiaoqingbo  PR title should also be changed.


-- 
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] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#discussion_r844947696


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/credentials/HadoopCredentialsManager.scala:
##########
@@ -183,6 +184,10 @@ class HadoopCredentialsManager private (name: String) extends AbstractService(na
           warn(
             s"Failed to send new credentials to SQL engine through session $sessionId",
             exception)
+          if (DELEGATION_TOKEN_IS_NOT_SUPPORTED.equals(exception.getMessage)) {
+            stop()

Review Comment:
   > It is an overkill to stop `HadoopCredentialManager` only because one of Kyuubi Engines does not support token renewal. This will result in token expiration of other Kyuubi Engines.
   > 
   > I think we should handle this more carefully in one of these two ways:
   > 
   > 1. At Kyuubi Server side, decide whether to `sendCredentials` according to `kyuubi.engine.type`.
   > 2. At Kyuubi Engine side, `TFrontendService#RenewDelegationToken` returns SUCCESS_STATUS by default.
   > 
   > By the first way, `HadoopCredentialManager` can maintain fewer userCredentials as currently only Spark Engine requires token renewal.
   > 
   > The second way conforms better to Kyuubi's architecture, that is, keep engine specific codes at engine side.
   
   You are correct, I thought kyuubi server can only support one engine at a time.
   Method 1 might work, method 2 would default to thinking that Engine supports it but actually just make useless rpc calls



-- 
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] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#discussion_r844947312


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/credentials/HadoopCredentialsManager.scala:
##########
@@ -183,6 +184,10 @@ class HadoopCredentialsManager private (name: String) extends AbstractService(na
           warn(
             s"Failed to send new credentials to SQL engine through session $sessionId",
             exception)
+          if (DELEGATION_TOKEN_IS_NOT_SUPPORTED.equals(exception.getMessage)) {
+            stop()

Review Comment:
   You are correct, I thought kyuubi server can only support one engine at a time.
   Method 1 might work, method 2 would default to thinking that Engine supports it but actually just make useless rpc calls



-- 
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] [incubator-kyuubi] codecov-commenter commented on pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#issuecomment-1094495143

   # [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286?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 [#2286](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (03caa96) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/0ee537108135ecf4a11cb144cc9134fffecdbc5c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0ee5371) will **increase** coverage by `0.00%`.
   > The diff coverage is `80.00%`.
   
   > :exclamation: Current head 03caa96 differs from pull request most recent head 83a9393. Consider uploading reports for the commit 83a9393 to get more accurate results
   
   ```diff
   @@            Coverage Diff            @@
   ##             master    #2286   +/-   ##
   =========================================
     Coverage     62.26%   62.27%           
     Complexity       69       69           
   =========================================
     Files           349      353    +4     
     Lines         16723    16771   +48     
     Branches       2261     2270    +9     
   =========================================
   + Hits          10413    10444   +31     
   - Misses         5354     5364   +10     
   - Partials        956      963    +7     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../apache/kyuubi/client/KyuubiSyncThriftClient.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jbGllbnQvS3l1dWJpU3luY1RocmlmdENsaWVudC5zY2FsYQ==) | `87.55% <66.66%> (-0.41%)` | :arrow_down: |
   | [...a/org/apache/kyuubi/service/TFrontendService.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2aWNlL1RGcm9udGVuZFNlcnZpY2Uuc2NhbGE=) | `93.35% <100.00%> (ø)` | |
   | [.../kyuubi/credentials/HadoopCredentialsManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jcmVkZW50aWFscy9IYWRvb3BDcmVkZW50aWFsc01hbmFnZXIuc2NhbGE=) | `94.77% <100.00%> (+1.64%)` | :arrow_up: |
   | [...g/apache/kyuubi/session/KyuubiSessionManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286/diff?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) | `79.48% <0.00%> (-15.39%)` | :arrow_down: |
   | [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvRW5naW5lUmVmLnNjYWxh) | `75.72% <0.00%> (-6.80%)` | :arrow_down: |
   | [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286/diff?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==) | `94.11% <0.00%> (-1.97%)` | :arrow_down: |
   | [...ache/kyuubi/engine/spark/SparkProcessBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvc3BhcmsvU3BhcmtQcm9jZXNzQnVpbGRlci5zY2FsYQ==) | `86.27% <0.00%> (-0.99%)` | :arrow_down: |
   | [...ugin/spark/authz/ranger/RangerSparkExtension.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L3Jhbmdlci9SYW5nZXJTcGFya0V4dGVuc2lvbi5zY2FsYQ==) | `100.00% <0.00%> (ø)` | |
   | [.../plugin/spark/authz/ranger/RangerSparkPlugin.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L3Jhbmdlci9SYW5nZXJTcGFya1BsdWdpbi5zY2FsYQ==) | | |
   | [...gin/spark/authz/ranger/RangerSparkAuthorizer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L3Jhbmdlci9SYW5nZXJTcGFya0F1dGhvcml6ZXIuc2NhbGE=) | | |
   | ... and [12 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [0ee5371...83a9393](https://codecov.io/gh/apache/incubator-kyuubi/pull/2286?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-kyuubi] yaooqinn commented on pull request #2286: [KYUUBI #2281] The RenewDelegationToken method of TFrontendService should return SUCCESS_STATUS by default

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#issuecomment-1096114011

   thanks, merged 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] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2286: [KYUUBI #2281] The RenewDelegationToken method of TFrontendService should return SUCCESS_STATUS by default

Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#discussion_r847871849


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/TFrontendService.scala:
##########
@@ -498,30 +498,29 @@ abstract class TFrontendService(name: String)
     resp
   }
 
-  private def notSupportTokenErrorStatus = {
-    val errorStatus = new TStatus(TStatusCode.ERROR_STATUS)
-    errorStatus.setErrorMessage("Delegation token is not supported")
-    errorStatus
+  private def supportTokenStatus = {
+    val successStatus = new TStatus(TStatusCode.SUCCESS_STATUS)
+    successStatus
   }
 
   override def GetDelegationToken(req: TGetDelegationTokenReq): TGetDelegationTokenResp = {
     debug(req.toString)
     val resp = new TGetDelegationTokenResp()
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)
     resp
   }
 
   override def CancelDelegationToken(req: TCancelDelegationTokenReq): TCancelDelegationTokenResp = {
     debug(req.toString)
     val resp = new TCancelDelegationTokenResp
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)
     resp
   }
 
   override def RenewDelegationToken(req: TRenewDelegationTokenReq): TRenewDelegationTokenResp = {
     debug(req.toString)
     val resp = new TRenewDelegationTokenResp
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)

Review Comment:
   I will change it



-- 
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] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#discussion_r846892282


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/credentials/HadoopCredentialsManager.scala:
##########
@@ -183,6 +184,10 @@ class HadoopCredentialsManager private (name: String) extends AbstractService(na
           warn(
             s"Failed to send new credentials to SQL engine through session $sessionId",
             exception)
+          if (DELEGATION_TOKEN_IS_NOT_SUPPORTED.equals(exception.getMessage)) {
+            stop()

Review Comment:
   > The option 2 makes more sense to me.
   
   The second is indeed more suitable, I will implement it



-- 
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] [incubator-kyuubi] yaooqinn closed pull request #2286: [KYUUBI #2281] The RenewDelegationToken method of TFrontendService should return SUCCESS_STATUS by default

Posted by GitBox <gi...@apache.org>.
yaooqinn closed pull request #2286: [KYUUBI #2281] The RenewDelegationToken method of TFrontendService should return SUCCESS_STATUS by default
URL: https://github.com/apache/incubator-kyuubi/pull/2286


-- 
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] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#discussion_r847214177


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/TFrontendService.scala:
##########
@@ -498,30 +498,29 @@ abstract class TFrontendService(name: String)
     resp
   }
 
-  private def notSupportTokenErrorStatus = {
-    val errorStatus = new TStatus(TStatusCode.ERROR_STATUS)
-    errorStatus.setErrorMessage("Delegation token is not supported")
-    errorStatus
+  private def supportTokenStatus = {
+    val successStatus = new TStatus(TStatusCode.SUCCESS_STATUS)
+    successStatus
   }
 
   override def GetDelegationToken(req: TGetDelegationTokenReq): TGetDelegationTokenResp = {
     debug(req.toString)
     val resp = new TGetDelegationTokenResp()
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)
     resp
   }
 
   override def CancelDelegationToken(req: TCancelDelegationTokenReq): TCancelDelegationTokenResp = {
     debug(req.toString)
     val resp = new TCancelDelegationTokenResp
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)
     resp
   }
 
   override def RenewDelegationToken(req: TRenewDelegationTokenReq): TRenewDelegationTokenResp = {
     debug(req.toString)
     val resp = new TRenewDelegationTokenResp
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)

Review Comment:
   we seem only need to change here?



-- 
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] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#discussion_r847213089


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/TFrontendService.scala:
##########
@@ -498,30 +498,29 @@ abstract class TFrontendService(name: String)
     resp
   }
 
-  private def notSupportTokenErrorStatus = {
-    val errorStatus = new TStatus(TStatusCode.ERROR_STATUS)
-    errorStatus.setErrorMessage("Delegation token is not supported")
-    errorStatus
+  private def supportTokenStatus = {
+    val successStatus = new TStatus(TStatusCode.SUCCESS_STATUS)
+    successStatus
   }
 
   override def GetDelegationToken(req: TGetDelegationTokenReq): TGetDelegationTokenResp = {
     debug(req.toString)
     val resp = new TGetDelegationTokenResp()
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)

Review Comment:
   OK_STATUS can be used here?



-- 
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] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#discussion_r847856646


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/TFrontendService.scala:
##########
@@ -498,30 +498,29 @@ abstract class TFrontendService(name: String)
     resp
   }
 
-  private def notSupportTokenErrorStatus = {
-    val errorStatus = new TStatus(TStatusCode.ERROR_STATUS)
-    errorStatus.setErrorMessage("Delegation token is not supported")
-    errorStatus
+  private def supportTokenStatus = {
+    val successStatus = new TStatus(TStatusCode.SUCCESS_STATUS)
+    successStatus
   }
 
   override def GetDelegationToken(req: TGetDelegationTokenReq): TGetDelegationTokenResp = {
     debug(req.toString)
     val resp = new TGetDelegationTokenResp()
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)
     resp
   }
 
   override def CancelDelegationToken(req: TCancelDelegationTokenReq): TCancelDelegationTokenResp = {
     debug(req.toString)
     val resp = new TCancelDelegationTokenResp
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)
     resp
   }
 
   override def RenewDelegationToken(req: TRenewDelegationTokenReq): TRenewDelegationTokenResp = {
     debug(req.toString)
     val resp = new TRenewDelegationTokenResp
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)

Review Comment:
   I will change it



-- 
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] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#discussion_r843927627


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/TFrontendService.scala:
##########
@@ -500,7 +500,7 @@ abstract class TFrontendService(name: String)
 
   private def notSupportTokenErrorStatus = {
     val errorStatus = new TStatus(TStatusCode.ERROR_STATUS)
-    errorStatus.setErrorMessage("Delegation token is not supported")
+    errorStatus.setErrorMessage(DELEGATION_TOKEN_IS_NOT_SUPPORTED)

Review Comment:
   Is this change necessary? Or you can define it at test 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] [incubator-kyuubi] zhouyifan279 commented on a diff in pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
zhouyifan279 commented on code in PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#discussion_r844818570


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/credentials/HadoopCredentialsManager.scala:
##########
@@ -183,6 +184,10 @@ class HadoopCredentialsManager private (name: String) extends AbstractService(na
           warn(
             s"Failed to send new credentials to SQL engine through session $sessionId",
             exception)
+          if (DELEGATION_TOKEN_IS_NOT_SUPPORTED.equals(exception.getMessage)) {
+            stop()

Review Comment:
   It is an overkill to stop `HadoopCredentialManager` only because one of Kyuubi Engines does not support token renewal. This will result in token expiration of other Kyuubi Engines.
   
   I think we should handle this more carefully in one of these two ways:
   
   1. At Kyuubi Server side, decide whether to `sendCredentials` according to `kyuubi.engine.type`.
   2. At Kyuubi Engine side, `TFrontendService#RenewDelegationToken` returns SUCCESS_STATUS by default.
   
   By the first way, `HadoopCredentialManager` can maintain fewer userCredentials as currently only Spark Engine requires token renewal.
   
   The second way conforms better to Kyuubi's architecture, that is, keep engine specific codes at engine 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] [incubator-kyuubi] jiaoqingbo commented on pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#issuecomment-1094585818

   cc @zhouyifan279  @pan3793 


-- 
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] [incubator-kyuubi] yaooqinn commented on pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#issuecomment-1094925736

   we shall still override this at kyuubi server for keep the old 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] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#discussion_r847215684


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/TFrontendService.scala:
##########
@@ -498,30 +498,29 @@ abstract class TFrontendService(name: String)
     resp
   }
 
-  private def notSupportTokenErrorStatus = {
-    val errorStatus = new TStatus(TStatusCode.ERROR_STATUS)
-    errorStatus.setErrorMessage("Delegation token is not supported")
-    errorStatus
+  private def supportTokenStatus = {
+    val successStatus = new TStatus(TStatusCode.SUCCESS_STATUS)
+    successStatus
   }
 
   override def GetDelegationToken(req: TGetDelegationTokenReq): TGetDelegationTokenResp = {
     debug(req.toString)
     val resp = new TGetDelegationTokenResp()
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)
     resp
   }
 
   override def CancelDelegationToken(req: TCancelDelegationTokenReq): TCancelDelegationTokenResp = {
     debug(req.toString)
     val resp = new TCancelDelegationTokenResp
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)
     resp
   }
 
   override def RenewDelegationToken(req: TRenewDelegationTokenReq): TRenewDelegationTokenResp = {
     debug(req.toString)
     val resp = new TRenewDelegationTokenResp
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)

Review Comment:
   OK_STATUS can be used here?



-- 
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] [incubator-kyuubi] zhouyifan279 commented on a diff in pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
zhouyifan279 commented on code in PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#discussion_r847328520


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/TFrontendService.scala:
##########
@@ -498,30 +498,29 @@ abstract class TFrontendService(name: String)
     resp
   }
 
-  private def notSupportTokenErrorStatus = {
-    val errorStatus = new TStatus(TStatusCode.ERROR_STATUS)
-    errorStatus.setErrorMessage("Delegation token is not supported")
-    errorStatus
+  private def supportTokenStatus = {
+    val successStatus = new TStatus(TStatusCode.SUCCESS_STATUS)
+    successStatus
   }
 
   override def GetDelegationToken(req: TGetDelegationTokenReq): TGetDelegationTokenResp = {
     debug(req.toString)
     val resp = new TGetDelegationTokenResp()
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)
     resp
   }
 
   override def CancelDelegationToken(req: TCancelDelegationTokenReq): TCancelDelegationTokenResp = {
     debug(req.toString)
     val resp = new TCancelDelegationTokenResp
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)
     resp
   }
 
   override def RenewDelegationToken(req: TRenewDelegationTokenReq): TRenewDelegationTokenResp = {
     debug(req.toString)
     val resp = new TRenewDelegationTokenResp
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)

Review Comment:
   `GetDelegationToken` and `CancelDelegationToken` should still return `notSupportTokenErrorStatus` as they have nothing to do with our token renewal.



-- 
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] [incubator-kyuubi] jiaoqingbo commented on pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#issuecomment-1095798715

   > we shall still override this at kyuubi server for keep the old behavior
   
   OK


-- 
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] [incubator-kyuubi] jiaoqingbo commented on pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#issuecomment-1091020602

   cc @zhouyifan279 @pan3793 
   


-- 
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] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#discussion_r843927627


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/TFrontendService.scala:
##########
@@ -500,7 +500,7 @@ abstract class TFrontendService(name: String)
 
   private def notSupportTokenErrorStatus = {
     val errorStatus = new TStatus(TStatusCode.ERROR_STATUS)
-    errorStatus.setErrorMessage("Delegation token is not supported")
+    errorStatus.setErrorMessage(DELEGATION_TOKEN_IS_NOT_SUPPORTED)

Review Comment:
   Is this change necessary?



-- 
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] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#discussion_r844558647


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/TFrontendService.scala:
##########
@@ -500,7 +500,7 @@ abstract class TFrontendService(name: String)
 
   private def notSupportTokenErrorStatus = {
     val errorStatus = new TStatus(TStatusCode.ERROR_STATUS)
-    errorStatus.setErrorMessage("Delegation token is not supported")
+    errorStatus.setErrorMessage(DELEGATION_TOKEN_IS_NOT_SUPPORTED)

Review Comment:
   > Is this change necessary? Or you can define it at test side
   
   I need to judge whether the Delegation token is not supported in the sendCredentials method of KyuubiSyncThriftClient.scala, if not set I need to use this string multiple times



-- 
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] [incubator-kyuubi] jiaoqingbo commented on a diff in pull request #2286: [KYUUBI #2281] renewalExecutor should be stopped when Engine's side Delegation token is not supported

Posted by GitBox <gi...@apache.org>.
jiaoqingbo commented on code in PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#discussion_r847871849


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/service/TFrontendService.scala:
##########
@@ -498,30 +498,29 @@ abstract class TFrontendService(name: String)
     resp
   }
 
-  private def notSupportTokenErrorStatus = {
-    val errorStatus = new TStatus(TStatusCode.ERROR_STATUS)
-    errorStatus.setErrorMessage("Delegation token is not supported")
-    errorStatus
+  private def supportTokenStatus = {
+    val successStatus = new TStatus(TStatusCode.SUCCESS_STATUS)
+    successStatus
   }
 
   override def GetDelegationToken(req: TGetDelegationTokenReq): TGetDelegationTokenResp = {
     debug(req.toString)
     val resp = new TGetDelegationTokenResp()
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)
     resp
   }
 
   override def CancelDelegationToken(req: TCancelDelegationTokenReq): TCancelDelegationTokenResp = {
     debug(req.toString)
     val resp = new TCancelDelegationTokenResp
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)
     resp
   }
 
   override def RenewDelegationToken(req: TRenewDelegationTokenReq): TRenewDelegationTokenResp = {
     debug(req.toString)
     val resp = new TRenewDelegationTokenResp
-    resp.setStatus(notSupportTokenErrorStatus)
+    resp.setStatus(supportTokenStatus)

Review Comment:
   TStatusCode has no value for OK_STATUS，and the sendCredentials method of KyuubiSyncThriftClient will determine whether the status is SUCCESS_STATUS。



-- 
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] [incubator-kyuubi] zhouyifan279 commented on pull request #2286: [KYUUBI #2281] The RenewDelegationToken method of TFrontendService should return SUCCESS_STATUS by default

Posted by GitBox <gi...@apache.org>.
zhouyifan279 commented on PR #2286:
URL: https://github.com/apache/incubator-kyuubi/pull/2286#issuecomment-1095829368

   > @jiaoqingbo PR title should also be changed. Besides, I think we should backport this PR to branch 1.5 as the bug causes other Kyuubi engine to fail to launch. @yaooqinn @pan3793 WDYT?
   
   Sorry, my mistake.  Branch 1.5 does not have this bug.


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