You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "turboFei (via GitHub)" <gi...@apache.org> on 2023/03/15 07:48:17 UTC

[GitHub] [kyuubi] turboFei opened a new pull request, #4521: [KYUUBI #3739][FOLLOWUP][REST] Remove unused parameters in SessionsResource rest docs

turboFei opened a new pull request, #4521:
URL: https://github.com/apache/kyuubi/pull/4521

   <!--
   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 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] turboFei commented on a diff in pull request #4521: [KYUUBI #3739][FOLLOWUP][REST] Remove unused parameters in SessionsResource rest docs

Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #4521:
URL: https://github.com/apache/kyuubi/pull/4521#discussion_r1137075868


##########
docs/deployment/migration-guide.md:
##########
@@ -24,7 +24,7 @@
 * Since Kyuubi 1.7, Kyuubi returns engine's information for `GetInfo` request instead of server. To restore the previous behavior, set `kyuubi.server.info.provider` to `SERVER`.
 * Since Kyuubi 1.7, Kyuubi session type `SQL` is refactored to `INTERACTIVE`, because Kyuubi supports not only `SQL` session, but also `SCALA` and `PYTHON` sessions.
   User need to use `INTERACTIVE` sessionType to look up the session event.
-* Since Kyuubi 1.7, the REST API of `Open(create) a session` will not contains parameters `user` `password` and `IpAddr`. User and password should be set in `Authorization` of http request if needed.
+* Since Kyuubi 1.7, the REST API of `Open(create) a session` will not contain parameters `protocolVersion`, `user` `password` and `IpAddr`. User and password should be set in `Authorization` of http request if needed.

Review Comment:
   Nothing happen. By default，protocol is 0， 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] yaooqinn commented on a diff in pull request #4521: [KYUUBI #3739][FOLLOWUP][REST] Remove unused parameters in SessionsResource rest docs

Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #4521:
URL: https://github.com/apache/kyuubi/pull/4521#discussion_r1138250088


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/SessionsResource.scala:
##########
@@ -137,7 +137,7 @@ private[v1] class SessionsResource extends ApiRequestContext with Logging {
     val userName = fe.getSessionUser(request.getConfigs.asScala.toMap)
     val ipAddress = fe.getIpAddress
     val handle = fe.be.openSession(
-      TProtocolVersion.findByValue(request.getProtocolVersion),
+      TProtocolVersion.values().max,

Review Comment:
   nit: make it a final val



-- 
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] yaooqinn commented on a diff in pull request #4521: [KYUUBI #3739][FOLLOWUP][REST] Remove unused parameters in SessionsResource rest docs

Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #4521:
URL: https://github.com/apache/kyuubi/pull/4521#discussion_r1138251745


##########
docs/deployment/migration-guide.md:
##########
@@ -24,7 +24,7 @@
 * Since Kyuubi 1.7, Kyuubi returns engine's information for `GetInfo` request instead of server. To restore the previous behavior, set `kyuubi.server.info.provider` to `SERVER`.
 * Since Kyuubi 1.7, Kyuubi session type `SQL` is refactored to `INTERACTIVE`, because Kyuubi supports not only `SQL` session, but also `SCALA` and `PYTHON` sessions.
   User need to use `INTERACTIVE` sessionType to look up the session event.
-* Since Kyuubi 1.7, the REST API of `Open(create) a session` will not contains parameters `user` `password` and `IpAddr`. User and password should be set in `Authorization` of http request if needed.
+* Since Kyuubi 1.7, the REST API of `Open(create) a session` will not contain parameters `protocolVersion`, `user` `password` and `IpAddr`. User and password should be set in `Authorization` of http request if needed.

Review Comment:
   We need doc it separately with patch-numbered releases, 1.7 looks aimless



-- 
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 #4521: [KYUUBI #3739][FOLLOWUP][REST] Remove unused parameters in SessionsResource rest docs

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #4521:
URL: https://github.com/apache/kyuubi/pull/4521#issuecomment-1469605266

   ## [Codecov](https://codecov.io/gh/apache/kyuubi/pull/4521?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 [#4521](https://codecov.io/gh/apache/kyuubi/pull/4521?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2960602) into [master](https://codecov.io/gh/apache/kyuubi/commit/e53da12419ec34dd134e80f2c96c9619e48cda7e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e53da12) will **increase** coverage by `0.02%`.
   > The diff coverage is `n/a`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #4521      +/-   ##
   ============================================
   + Coverage     53.34%   53.37%   +0.02%     
     Complexity       13       13              
   ============================================
     Files           571      571              
     Lines         31333    31350      +17     
     Branches       4223     4225       +2     
   ============================================
   + Hits          16715    16733      +18     
   - Misses        13038    13041       +3     
   + Partials       1580     1576       -4     
   ```
   
   
   [see 9 files with indirect coverage changes](https://codecov.io/gh/apache/kyuubi/pull/4521/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 #4521: [KYUUBI #3739][FOLLOWUP][REST] Remove unused parameters in SessionsResource rest docs

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4521:
URL: https://github.com/apache/kyuubi/pull/4521#discussion_r1138296966


##########
docs/deployment/migration-guide.md:
##########
@@ -17,14 +17,18 @@
 
 # Kyuubi Migration Guide
 
+## Upgrading from Kyuubi 1.7.0 to 1.7.1
+
+* Since Kyuubi 1.7.1, the REST API of `Open(create) a session` will not contain parameters `protocolVersion`.

Review Comment:
   I think we should clear document it does not break user to use old REST client to connect Kyuubi Server.
   ```
   Since Kyuubi 1.7.1, `protocolVersion` is removed from the request parameters of the REST API `Open(create) a session`. All removed or unknown parameters will be silently ignored and affects nothing.
   ```



##########
docs/deployment/migration-guide.md:
##########
@@ -17,14 +17,18 @@
 
 # Kyuubi Migration Guide
 
+## Upgrading from Kyuubi 1.7.0 to 1.7.1
+
+* Since Kyuubi 1.7.1, the REST API of `Open(create) a session` will not contain parameters `protocolVersion`.

Review Comment:
   I think we should clearly document it does not break user to use old REST client to connect Kyuubi Server.
   ```
   Since Kyuubi 1.7.1, `protocolVersion` is removed from the request parameters of the REST API `Open(create) a session`. All removed or unknown parameters will be silently ignored and affects nothing.
   ```



-- 
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 #4521: [KYUUBI #3739][FOLLOWUP][REST] Remove unused parameters in SessionsResource rest docs

Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei closed pull request #4521: [KYUUBI #3739][FOLLOWUP][REST] Remove unused parameters in SessionsResource rest docs
URL: https://github.com/apache/kyuubi/pull/4521


-- 
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 #4521: [KYUUBI #3739][FOLLOWUP][REST] Remove unused parameters in SessionsResource rest docs

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4521:
URL: https://github.com/apache/kyuubi/pull/4521#discussion_r1137038346


##########
docs/deployment/migration-guide.md:
##########
@@ -24,7 +24,7 @@
 * Since Kyuubi 1.7, Kyuubi returns engine's information for `GetInfo` request instead of server. To restore the previous behavior, set `kyuubi.server.info.provider` to `SERVER`.
 * Since Kyuubi 1.7, Kyuubi session type `SQL` is refactored to `INTERACTIVE`, because Kyuubi supports not only `SQL` session, but also `SCALA` and `PYTHON` sessions.
   User need to use `INTERACTIVE` sessionType to look up the session event.
-* Since Kyuubi 1.7, the REST API of `Open(create) a session` will not contains parameters `user` `password` and `IpAddr`. User and password should be set in `Authorization` of http request if needed.
+* Since Kyuubi 1.7, the REST API of `Open(create) a session` will not contain parameters `protocolVersion`, `user` `password` and `IpAddr`. User and password should be set in `Authorization` of http request if needed.

Review Comment:
   question: what will happen if a user uses Kyuubi 1.7.0 client access Kyuubi Server 1.7.1



-- 
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 #4521: [KYUUBI #3739][FOLLOWUP][REST] Remove unused parameters in SessionsResource rest docs

Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #4521:
URL: https://github.com/apache/kyuubi/pull/4521#discussion_r1137900979


##########
docs/deployment/migration-guide.md:
##########
@@ -24,7 +24,7 @@
 * Since Kyuubi 1.7, Kyuubi returns engine's information for `GetInfo` request instead of server. To restore the previous behavior, set `kyuubi.server.info.provider` to `SERVER`.
 * Since Kyuubi 1.7, Kyuubi session type `SQL` is refactored to `INTERACTIVE`, because Kyuubi supports not only `SQL` session, but also `SCALA` and `PYTHON` sessions.
   User need to use `INTERACTIVE` sessionType to look up the session event.
-* Since Kyuubi 1.7, the REST API of `Open(create) a session` will not contains parameters `user` `password` and `IpAddr`. User and password should be set in `Authorization` of http request if needed.
+* Since Kyuubi 1.7, the REST API of `Open(create) a session` will not contain parameters `protocolVersion`, `user` `password` and `IpAddr`. User and password should be set in `Authorization` of http request if needed.

Review Comment:
   The unknown field should be ignore.



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