You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "ulysses-you (via GitHub)" <gi...@apache.org> on 2023/02/14 04:15:13 UTC
[GitHub] [kyuubi] ulysses-you opened a new pull request, #4323: Improve trino session context
ulysses-you opened a new pull request, #4323:
URL: https://github.com/apache/kyuubi/pull/4323
<!--
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.
-->
This pr improves the trino session context:
1. always reuse the kyuubi session if session id exists, so we can restore the session context for next query
2. transform trino client information to kyuubi session, e.g. trino request source (trino-cli)
### _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] ulysses-you commented on pull request #4323: Improve trino session context
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on PR #4323:
URL: https://github.com/apache/kyuubi/pull/4323#issuecomment-1429259243
thanks, merging to master/1.7
--
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 #4323: Improve trino session context
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #4323:
URL: https://github.com/apache/kyuubi/pull/4323#issuecomment-1429150625
# [Codecov](https://codecov.io/gh/apache/kyuubi/pull/4323?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 [#4323](https://codecov.io/gh/apache/kyuubi/pull/4323?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (59804ba) into [master](https://codecov.io/gh/apache/kyuubi/commit/4de0a697ae700d5c6d6122f740437f38d66715dd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4de0a69) will **increase** coverage by `0.01%`.
> The diff coverage is `94.11%`.
```diff
@@ Coverage Diff @@
## master #4323 +/- ##
============================================
+ Coverage 53.65% 53.66% +0.01%
Complexity 13 13
============================================
Files 562 562
Lines 30731 30746 +15
Branches 4159 4159
============================================
+ Hits 16488 16501 +13
- Misses 12686 12687 +1
- Partials 1557 1558 +1
```
| [Impacted Files](https://codecov.io/gh/apache/kyuubi/pull/4323?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ala/org/apache/kyuubi/server/trino/api/Query.scala](https://codecov.io/gh/apache/kyuubi/pull/4323?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvdHJpbm8vYXBpL1F1ZXJ5LnNjYWxh) | `83.63% <92.30%> (+0.65%)` | :arrow_up: |
| [.../apache/kyuubi/server/trino/api/TrinoContext.scala](https://codecov.io/gh/apache/kyuubi/pull/4323?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvdHJpbm8vYXBpL1RyaW5vQ29udGV4dC5zY2FsYQ==) | `58.33% <100.00%> (+1.19%)` | :arrow_up: |
| [...kyuubi/server/trino/api/v1/StatementResource.scala](https://codecov.io/gh/apache/kyuubi/pull/4323?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvdHJpbm8vYXBpL3YxL1N0YXRlbWVudFJlc291cmNlLnNjYWxh) | `50.00% <100.00%> (+1.35%)` | :arrow_up: |
| [...apache/kyuubi/engine/JpsApplicationOperation.scala](https://codecov.io/gh/apache/kyuubi/pull/4323?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvSnBzQXBwbGljYXRpb25PcGVyYXRpb24uc2NhbGE=) | `77.41% <0.00%> (-3.23%)` | :arrow_down: |
| [...g/apache/kyuubi/operation/BatchJobSubmission.scala](https://codecov.io/gh/apache/kyuubi/pull/4323?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) | `75.27% <0.00%> (-2.20%)` | :arrow_down: |
| [...mon/src/main/scala/org/apache/kyuubi/Logging.scala](https://codecov.io/gh/apache/kyuubi/pull/4323?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9Mb2dnaW5nLnNjYWxh) | `41.25% <0.00%> (-1.25%)` | :arrow_down: |
| [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/kyuubi/pull/4323?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvY0J1aWxkZXIuc2NhbGE=) | `78.39% <0.00%> (-0.62%)` | :arrow_down: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/kyuubi/pull/4323?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.44% <0.00%> (-0.07%)` | :arrow_down: |
| [...kyuubi/engine/KubernetesApplicationOperation.scala](https://codecov.io/gh/apache/kyuubi/pull/4323?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvS3ViZXJuZXRlc0FwcGxpY2F0aW9uT3BlcmF0aW9uLnNjYWxh) | `22.53% <0.00%> (ø)` | |
| [...g/apache/kyuubi/session/KyuubiSessionManager.scala](https://codecov.io/gh/apache/kyuubi/pull/4323?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) | `94.89% <0.00%> (+0.72%)` | :arrow_up: |
| ... and [3 more](https://codecov.io/gh/apache/kyuubi/pull/4323?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] ulysses-you closed pull request #4323: Improve trino session context
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you closed pull request #4323: Improve trino session context
URL: https://github.com/apache/kyuubi/pull/4323
--
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] iodone commented on a diff in pull request #4323: Improve trino session context
Posted by "iodone (via GitHub)" <gi...@apache.org>.
iodone commented on code in PR #4323:
URL: https://github.com/apache/kyuubi/pull/4323#discussion_r1105370633
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/TrinoContext.scala:
##########
@@ -140,15 +142,17 @@ object TrinoContext {
def buildTrinoResponse(qr: QueryResults, trinoContext: TrinoContext): Response = {
val responseBuilder = Response.ok(qr)
- trinoContext.catalog.foreach(
- responseBuilder.header(TRINO_HEADERS.responseSetCatalog, _))
- trinoContext.schema.foreach(
- responseBuilder.header(TRINO_HEADERS.responseSetSchema, _))
+ // Note, We have injected kyuubi session id to session context so that the next query can find
+ // the previous session to restore the query context.
+ // It's hard to follow the Trino style that set all context to http headers.
+ // Because we do not know the context at server side. e.g. `set k=v`, `use database`.
+ // We also can not inject other session context into header before we supporting to map
+ // query result to session context.
Review Comment:
Instead of returning all session information to the client with HTTP response header, set it to sessionConfig and use kyuubi session to save the session information 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] ulysses-you commented on a diff in pull request #4323: Improve trino session context
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4323:
URL: https://github.com/apache/kyuubi/pull/4323#discussion_r1105378462
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/TrinoContext.scala:
##########
@@ -140,15 +142,17 @@ object TrinoContext {
def buildTrinoResponse(qr: QueryResults, trinoContext: TrinoContext): Response = {
val responseBuilder = Response.ok(qr)
- trinoContext.catalog.foreach(
- responseBuilder.header(TRINO_HEADERS.responseSetCatalog, _))
- trinoContext.schema.foreach(
- responseBuilder.header(TRINO_HEADERS.responseSetSchema, _))
+ // Note, We have injected kyuubi session id to session context so that the next query can find
+ // the previous session to restore the query context.
+ // It's hard to follow the Trino style that set all context to http headers.
+ // Because we do not know the context at server side. e.g. `set k=v`, `use database`.
+ // We also can not inject other session context into header before we supporting to map
+ // query result to session context.
Review Comment:
yea, I think it's not the trino way but more suitable for Kyuubi
--
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] iodone commented on a diff in pull request #4323: Improve trino session context
Posted by "iodone (via GitHub)" <gi...@apache.org>.
iodone commented on code in PR #4323:
URL: https://github.com/apache/kyuubi/pull/4323#discussion_r1105391333
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/TrinoContext.scala:
##########
@@ -140,15 +142,17 @@ object TrinoContext {
def buildTrinoResponse(qr: QueryResults, trinoContext: TrinoContext): Response = {
val responseBuilder = Response.ok(qr)
- trinoContext.catalog.foreach(
- responseBuilder.header(TRINO_HEADERS.responseSetCatalog, _))
- trinoContext.schema.foreach(
- responseBuilder.header(TRINO_HEADERS.responseSetSchema, _))
+ // Note, We have injected kyuubi session id to session context so that the next query can find
+ // the previous session to restore the query context.
+ // It's hard to follow the Trino style that set all context to http headers.
+ // Because we do not know the context at server side. e.g. `set k=v`, `use database`.
+ // We also can not inject other session context into header before we supporting to map
+ // query result to session context.
Review Comment:
Nice, elegant solutions
--
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] ulysses-you commented on a diff in pull request #4323: Improve trino session context
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4323:
URL: https://github.com/apache/kyuubi/pull/4323#discussion_r1105274330
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/TrinoContext.scala:
##########
@@ -140,15 +142,17 @@ object TrinoContext {
def buildTrinoResponse(qr: QueryResults, trinoContext: TrinoContext): Response = {
val responseBuilder = Response.ok(qr)
- trinoContext.catalog.foreach(
- responseBuilder.header(TRINO_HEADERS.responseSetCatalog, _))
- trinoContext.schema.foreach(
- responseBuilder.header(TRINO_HEADERS.responseSetSchema, _))
+ // Note, We have injected kyuubi session id to session context so that the next query can find
+ // the previous session to restore the query context.
+ // It's hard to follow the Trino style that set all context to http headers.
+ // Because we do not know the context at server side. e.g. `set k=v`, `use database`.
+ // We also can not inject other session context into header before we supporting to map
+ // query result to session context.
Review Comment:
cc @iodone
We can not inject session context using the current request header. e.g. the current query is `set k=v2` but he exists request header is `k=v`. So we only inject kyuubi session id to help next query find session context.
--
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