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/06/02 08:48:46 UTC
[GitHub] [incubator-kyuubi] cxzl25 opened a new pull request, #2808: [KYUUBI #2807] Expose sessionKyuubiConf in Session
cxzl25 opened a new pull request, #2808:
URL: https://github.com/apache/incubator-kyuubi/pull/2808
### _Why are the changes needed?_
In `OperationManager` we use `session.sessionManager.getConf` to check if some configuration is enabled, but ignore session level configuration.
Supporting session conf also helps with testing `withSessionConf`.
close #2807
### _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] ulysses-you closed pull request #2808: [KYUUBI #2807] Trino, Hive and JDBC Engine support session conf in newExecuteStatementOperation
Posted by GitBox <gi...@apache.org>.
ulysses-you closed pull request #2808: [KYUUBI #2807] Trino, Hive and JDBC Engine support session conf in newExecuteStatementOperation
URL: https://github.com/apache/incubator-kyuubi/pull/2808
--
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 #2808: [KYUUBI #2807] Expose sessionKyuubiConf in Session
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2808:
URL: https://github.com/apache/incubator-kyuubi/pull/2808#issuecomment-1144688294
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808?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 [#2808](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d0f4798) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/bb98aa75e48672927ee5b6ed873d12b4fde1f07e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bb98aa7) will **increase** coverage by `0.05%`.
> The diff coverage is `66.66%`.
> :exclamation: Current head d0f4798 differs from pull request most recent head 3e7ae43. Consider uploading reports for the commit 3e7ae43 to get more accurate results
```diff
@@ Coverage Diff @@
## master #2808 +/- ##
============================================
+ Coverage 62.92% 62.98% +0.05%
Complexity 202 202
============================================
Files 481 481
Lines 21415 21421 +6
Branches 2843 2843
============================================
+ Hits 13475 13491 +16
+ Misses 6663 6647 -16
- Partials 1277 1283 +6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...i/engine/hive/operation/HiveOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808/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-ZXh0ZXJuYWxzL2t5dXViaS1oaXZlLXNxbC1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL2hpdmUvb3BlcmF0aW9uL0hpdmVPcGVyYXRpb25NYW5hZ2VyLnNjYWxh) | `93.47% <0.00%> (ø)` | |
| [...la/org/apache/kyuubi/session/AbstractSession.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0Fic3RyYWN0U2Vzc2lvbi5zY2FsYQ==) | `95.49% <60.00%> (-1.68%)` | :arrow_down: |
| [...kyuubi/engine/trino/operation/TrinoOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808/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-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL29wZXJhdGlvbi9Ucmlub09wZXJhdGlvbi5zY2FsYQ==) | `70.00% <100.00%> (+14.89%)` | :arrow_up: |
| [...engine/trino/operation/TrinoOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808/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-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL29wZXJhdGlvbi9Ucmlub09wZXJhdGlvbk1hbmFnZXIuc2NhbGE=) | `88.57% <100.00%> (+2.85%)` | :arrow_up: |
| [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808/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==) | `80.55% <0.00%> (-1.39%)` | :arrow_down: |
| [.../org/apache/kyuubi/operation/KyuubiOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uLnNjYWxh) | `68.00% <0.00%> (-1.34%)` | :arrow_down: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808/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) | `76.92% <0.00%> (-0.97%)` | :arrow_down: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `96.94% <0.00%> (+0.10%)` | :arrow_up: |
| [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvY0J1aWxkZXIuc2NhbGE=) | `84.90% <0.00%> (+0.62%)` | :arrow_up: |
| [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808/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-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL1RyaW5vU3RhdGVtZW50LnNjYWxh) | `77.96% <0.00%> (+8.47%)` | :arrow_up: |
| ... and [1 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808/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/2808?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/2808?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 [bb98aa7...3e7ae43](https://codecov.io/gh/apache/incubator-kyuubi/pull/2808?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] cxzl25 commented on a diff in pull request #2808: [KYUUBI #2807] Expose sessionKyuubiConf in Session
Posted by GitBox <gi...@apache.org>.
cxzl25 commented on code in PR #2808:
URL: https://github.com/apache/incubator-kyuubi/pull/2808#discussion_r888166468
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/session/Session.scala:
##########
@@ -29,6 +30,8 @@ trait Session {
def conf: Map[String, String]
+ def sessionKyuubiConf: KyuubiConf
Review Comment:
> The name looks too long, what about renaming `conf` to `confOverlay`, and naming it `conf`?
I see that there are several configurations in the session, `conf`, `sessionConf`, `normalizedConf`.
--
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] ulysses-you commented on pull request #2808: [KYUUBI #2807] Trino, Hive and JDBC Engine support session conf in newExecuteStatementOperation
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #2808:
URL: https://github.com/apache/incubator-kyuubi/pull/2808#issuecomment-1146951335
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] [incubator-kyuubi] pan3793 commented on a diff in pull request #2808: [KYUUBI #2807] Expose sessionKyuubiConf in Session
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2808:
URL: https://github.com/apache/incubator-kyuubi/pull/2808#discussion_r888145264
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/session/Session.scala:
##########
@@ -29,6 +30,8 @@ trait Session {
def conf: Map[String, String]
+ def sessionKyuubiConf: KyuubiConf
Review Comment:
The name looks too long, what about renaming `conf` to `confOverlay`, and naming it `conf`? cc @ulysses-you
--
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] ulysses-you commented on a diff in pull request #2808: [KYUUBI #2807] Expose sessionKyuubiConf in Session
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #2808:
URL: https://github.com/apache/incubator-kyuubi/pull/2808#discussion_r888577769
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/session/Session.scala:
##########
@@ -29,6 +30,8 @@ trait Session {
def conf: Map[String, String]
+ def sessionKyuubiConf: KyuubiConf
Review Comment:
The session conf pipeline is:
```
user request conf(conf in Session) -> normalized conf -> advisor conf
```
Some session implementation have already merged the session conf to KyuubiConf. e.g. `KyuubiSessionImpl`, so it's not correct to merge normalized conf to kyuubi conf.
I'd like to only do fix the trino and hive operation manager that use normalized conf instead of global kyuubi conf.
--
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] pan3793 commented on a diff in pull request #2808: [KYUUBI #2807] Expose sessionKyuubiConf in Session
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2808:
URL: https://github.com/apache/incubator-kyuubi/pull/2808#discussion_r888146621
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/session/Session.scala:
##########
@@ -29,6 +30,8 @@ trait Session {
def conf: Map[String, String]
+ def sessionKyuubiConf: KyuubiConf
Review Comment:
Not related: `def closeExpiredOperations: Unit` has side effect, let's add a `()` on the method
--
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