You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "zwangsheng (via GitHub)" <gi...@apache.org> on 2023/04/06 08:38:54 UTC
[GitHub] [kyuubi] zwangsheng opened a new pull request, #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
zwangsheng opened a new pull request, #4670:
URL: https://github.com/apache/kyuubi/pull/4670
<!--
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.
-->
Add List Kyuubi Server Api for `AdminResource`
### _How was this patch tested?_
- [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [ ] [Run test](https://kyuubi.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] zwangsheng commented on pull request #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on PR #4670:
URL: https://github.com/apache/kyuubi/pull/4670#issuecomment-1498694802
cc @turboFei @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] [kyuubi] turboFei commented on pull request #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #4670:
URL: https://github.com/apache/kyuubi/pull/4670#issuecomment-1498799451
some comments:
1. we can support `discoveryService` for KyuubiRestFrontendService
2. we shall return the rest url for the API in this pr
--
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 #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #4670:
URL: https://github.com/apache/kyuubi/pull/4670#issuecomment-1498948666
## [Codecov](https://codecov.io/gh/apache/kyuubi/pull/4670?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 [#4670](https://codecov.io/gh/apache/kyuubi/pull/4670?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4271d0f) into [master](https://codecov.io/gh/apache/kyuubi/commit/1241a3891431cda602269bd49dcb8621ad77bbb5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1241a38) will **decrease** coverage by `0.02%`.
> The diff coverage is `55.73%`.
```diff
@@ Coverage Diff @@
## master #4670 +/- ##
============================================
- Coverage 57.61% 57.59% -0.02%
Complexity 13 13
============================================
Files 579 580 +1
Lines 31966 32027 +61
Branches 4270 4278 +8
============================================
+ Hits 18417 18446 +29
- Misses 11785 11814 +29
- Partials 1764 1767 +3
```
| [Impacted Files](https://codecov.io/gh/apache/kyuubi/pull/4670?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...rg/apache/kyuubi/client/api/v1/dto/ServerData.java](https://codecov.io/gh/apache/kyuubi/pull/4670?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXJlc3QtY2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9reXV1YmkvY2xpZW50L2FwaS92MS9kdG8vU2VydmVyRGF0YS5qYXZh) | `40.00% <40.00%> (ø)` | |
| [...rg/apache/kyuubi/server/api/v1/AdminResource.scala](https://codecov.io/gh/apache/kyuubi/pull/4670?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvYXBpL3YxL0FkbWluUmVzb3VyY2Uuc2NhbGE=) | `84.71% <76.92%> (-0.71%)` | :arrow_down: |
| [.../scala/org/apache/kyuubi/server/api/ApiUtils.scala](https://codecov.io/gh/apache/kyuubi/pull/4670?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvYXBpL0FwaVV0aWxzLnNjYWxh) | `100.00% <100.00%> (ø)` | |
... and [8 files with indirect coverage changes](https://codecov.io/gh/apache/kyuubi/pull/4670/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] zwangsheng commented on pull request #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on PR #4670:
URL: https://github.com/apache/kyuubi/pull/4670#issuecomment-1501584649
> BTW, I recommend to support discovery service for KyuubiRestFrontendService.
> And the URLs return in the api added in this PR should be the connection urls of `KyuubiRestFrontendService`s.
I agree with support discovery service for `KyuubiRestFrontendService` and expose rest connection url in HA node.
Seems this is a different goal feature, i'd like make a new PR to implement.
--
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 #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #4670:
URL: https://github.com/apache/kyuubi/pull/4670#issuecomment-1512466676
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] turboFei closed pull request #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei closed pull request #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
URL: https://github.com/apache/kyuubi/pull/4670
--
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 #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on PR #4670:
URL: https://github.com/apache/kyuubi/pull/4670#issuecomment-1512334234
ping @pan3793 @turboFei, we can merge this.
--
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 a diff in pull request #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on code in PR #4670:
URL: https://github.com/apache/kyuubi/pull/4670#discussion_r1160378890
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/AdminResourceSuite.scala:
##########
@@ -488,4 +491,41 @@ class AdminResourceSuite extends KyuubiFunSuite with RestFrontendTestHelper {
}
}
}
+
+ test("list server") {
+ // Mock Kyuubi Server
+ val serverDiscovery = mock[ServiceDiscovery]
+ lenient.when(serverDiscovery.fe).thenReturn(fe)
+ val namespace = conf.get(HighAvailabilityConf.HA_NAMESPACE)
+ withDiscoveryClient(conf) { client =>
+ client.registerService(conf, namespace, serverDiscovery)
+
+ val adminUser = Utils.currentUser
+ val encodeAuthorization = new String(
+ Base64.getEncoder.encode(
+ s"$adminUser:".getBytes()),
+ "UTF-8")
Review Comment:
Thanks for point this.
By the way, other similar places in this document have been modified and duplicate code has been reduced.
--
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 #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on PR #4670:
URL: https://github.com/apache/kyuubi/pull/4670#issuecomment-1498866860
> some comments:
>
> 1. we can support `discoveryService` for KyuubiRestFrontendService
> 2. we shall return the rest fe urls for the API in this pr
@turboFei
Sure, made changes.
--
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 #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #4670:
URL: https://github.com/apache/kyuubi/pull/4670#discussion_r1160366853
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/AdminResourceSuite.scala:
##########
@@ -488,4 +491,41 @@ class AdminResourceSuite extends KyuubiFunSuite with RestFrontendTestHelper {
}
}
}
+
+ test("list server") {
+ // Mock Kyuubi Server
+ val serverDiscovery = mock[ServiceDiscovery]
+ lenient.when(serverDiscovery.fe).thenReturn(fe)
+ val namespace = conf.get(HighAvailabilityConf.HA_NAMESPACE)
+ withDiscoveryClient(conf) { client =>
+ client.registerService(conf, namespace, serverDiscovery)
+
+ val adminUser = Utils.currentUser
+ val encodeAuthorization = new String(
+ Base64.getEncoder.encode(
+ s"$adminUser:".getBytes()),
+ "UTF-8")
Review Comment:
```suggestion
StandardCharsets.UTF_8)
```
--
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 #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4670:
URL: https://github.com/apache/kyuubi/pull/4670#discussion_r1161380711
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/AdminResource.scala:
##########
@@ -296,6 +296,35 @@ private[v1] class AdminResource extends ApiRequestContext with Logging {
node.attributes.asJava))
}
+ @ApiResponse(
+ responseCode = "200",
+ content = Array(
+ new Content(
+ mediaType = MediaType.APPLICATION_JSON,
+ array = new ArraySchema(schema = new Schema(implementation =
+ classOf[OperationData])))),
+ description = "list all live kyuubi servers")
+ @GET
+ @Path("server")
+ def listServers(): Seq[ServerData] = {
+ val userName = fe.getSessionUser(Map.empty[String, String])
+ val ipAddress = fe.getIpAddress
+ info(s"Received list all live kyuubi servers request from $userName/$ipAddress")
+ if (!isAdministrator(userName)) {
+ throw new NotAllowedException(
+ s"$userName is not allowed to list all live kyuubi servers")
+ }
+ val kyuubiConf = fe.getConf
+ val servers = ListBuffer[ServerData]()
+ val serverSpec = DiscoveryPaths.makePath(null, kyuubiConf.get(HA_NAMESPACE))
+ withDiscoveryClient(kyuubiConf) { discoveryClient =>
Review Comment:
what will happen if HA is disabled
--
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 #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on PR #4670:
URL: https://github.com/apache/kyuubi/pull/4670#issuecomment-1501295195
Also cc @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] [kyuubi] zwangsheng commented on a diff in pull request #4670: [KYUUBI #3653][REST] AdminResource add list kyuubi server api
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on code in PR #4670:
URL: https://github.com/apache/kyuubi/pull/4670#discussion_r1161383242
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/AdminResource.scala:
##########
@@ -296,6 +296,35 @@ private[v1] class AdminResource extends ApiRequestContext with Logging {
node.attributes.asJava))
}
+ @ApiResponse(
+ responseCode = "200",
+ content = Array(
+ new Content(
+ mediaType = MediaType.APPLICATION_JSON,
+ array = new ArraySchema(schema = new Schema(implementation =
+ classOf[OperationData])))),
+ description = "list all live kyuubi servers")
+ @GET
+ @Path("server")
+ def listServers(): Seq[ServerData] = {
+ val userName = fe.getSessionUser(Map.empty[String, String])
+ val ipAddress = fe.getIpAddress
+ info(s"Received list all live kyuubi servers request from $userName/$ipAddress")
+ if (!isAdministrator(userName)) {
+ throw new NotAllowedException(
+ s"$userName is not allowed to list all live kyuubi servers")
+ }
+ val kyuubiConf = fe.getConf
+ val servers = ListBuffer[ServerData]()
+ val serverSpec = DiscoveryPaths.makePath(null, kyuubiConf.get(HA_NAMESPACE))
+ withDiscoveryClient(kyuubiConf) { discoveryClient =>
Review Comment:
When disabled HA, `Kyuubi Server` will register self to `EmbeddedZookeeper`.
It's predictably to get only itself when disabled HA.
--
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