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/24 12:10:47 UTC
[GitHub] [incubator-kyuubi] wForget opened a new pull request, #2462: [KYUUBI #2461] Use the original host argument
wForget opened a new pull request, #2462:
URL: https://github.com/apache/incubator-kyuubi/pull/2462
<!--
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.
-->
close #2461
### _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] cxzl25 commented on a diff in pull request #2462: [KYUUBI #2461] Use the original host argument
Posted by GitBox <gi...@apache.org>.
cxzl25 commented on code in PR #2462:
URL: https://github.com/apache/incubator-kyuubi/pull/2462#discussion_r857118814
##########
kyuubi-ctl/src/main/scala/org/apache/kyuubi/ctl/ServiceControlCliArguments.scala:
##########
@@ -249,7 +249,8 @@ class ServiceControlCliArguments(args: Seq[String], env: Map[String, String] = s
}
try {
- cliArgs = cliArgs.copy(host = InetAddress.getByName(cliArgs.host).getCanonicalHostName)
+ InetAddress.getByName(cliArgs.host)
Review Comment:
Because `kyuubi.frontend.connection.url.use.hostname` is configured to determine whether the node registered to zookeeper is hostname or ip, would it be better to use the original value passed in with `--host` 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] ulysses-you closed pull request #2462: [KYUUBI #2461] Use the original host argument
Posted by GitBox <gi...@apache.org>.
ulysses-you closed pull request #2462: [KYUUBI #2461] Use the original host argument
URL: https://github.com/apache/incubator-kyuubi/pull/2462
--
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 #2462: [KYUUBI #2461] Use the original host argument
Posted by GitBox <gi...@apache.org>.
cxzl25 commented on code in PR #2462:
URL: https://github.com/apache/incubator-kyuubi/pull/2462#discussion_r857120223
##########
kyuubi-ctl/src/main/scala/org/apache/kyuubi/ctl/ServiceControlCliArguments.scala:
##########
@@ -249,7 +249,8 @@ class ServiceControlCliArguments(args: Seq[String], env: Map[String, String] = s
}
try {
- cliArgs = cliArgs.copy(host = InetAddress.getByName(cliArgs.host).getCanonicalHostName)
+ InetAddress.getByName(cliArgs.host)
Review Comment:
The server is configured with `kyuubi.frontend.thrift.binary.bind.host`, so `kyuubi.frontend.connection.url.use.hostname` does not take effect.
https://github.com/apache/incubator-kyuubi/blob/deb0e6203b5b56a1c61162f793d3601be0395118/kyuubi-common/src/main/scala/org/apache/kyuubi/service/TFrontendService.scala#L109-L119
Because there may be the ip used by the server and the hostname used by the engine, it is sometimes embarrassing to convert `--host`.
--
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 #2462: [KYUUBI #2461] Use the original host argument
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #2462:
URL: https://github.com/apache/incubator-kyuubi/pull/2462#issuecomment-1108010327
I guess @cxzl25 also misunderstand the behavior according to the comments, isn't @cxzl25 ?
--
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] wForget commented on pull request #2462: [KYUUBI #2461] Use the original host argument
Posted by GitBox <gi...@apache.org>.
wForget commented on PR #2462:
URL: https://github.com/apache/incubator-kyuubi/pull/2462#issuecomment-1107997027
`InetAddress.getByName(cliArgs.host)` is only used to validate the host argument.
--
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 #2462: [KYUUBI #2461] Use the original host argument
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #2462:
URL: https://github.com/apache/incubator-kyuubi/pull/2462#issuecomment-1108044490
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] wForget commented on a diff in pull request #2462: [KYUUBI #2461] Use the original host argument
Posted by GitBox <gi...@apache.org>.
wForget commented on code in PR #2462:
URL: https://github.com/apache/incubator-kyuubi/pull/2462#discussion_r857119434
##########
kyuubi-ctl/src/main/scala/org/apache/kyuubi/ctl/ServiceControlCliArguments.scala:
##########
@@ -249,7 +249,8 @@ class ServiceControlCliArguments(args: Seq[String], env: Map[String, String] = s
}
try {
- cliArgs = cliArgs.copy(host = InetAddress.getByName(cliArgs.host).getCanonicalHostName)
+ InetAddress.getByName(cliArgs.host)
Review Comment:
> Because `kyuubi.frontend.connection.url.use.hostname` is configured to determine whether the node registered to zookeeper is hostname or ip, would it be better to use the original value passed in with `--host` here?
When we configure `kyuubi.frontend.thrift.binary.bind.host`, the `kyuubi.frontend.connection.url.use.hostname` config does not seem to take effect, we use the original host argument will be more flexible.
--
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] wForget commented on pull request #2462: [KYUUBI #2461] Use the original host argument
Posted by GitBox <gi...@apache.org>.
wForget commented on PR #2462:
URL: https://github.com/apache/incubator-kyuubi/pull/2462#issuecomment-1107995751
> shall we just pass the input string which respect user specified ? It seems it's unnecessary to parse it to hostname
I think this should be done, this pr is like 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] [incubator-kyuubi] codecov-commenter commented on pull request #2462: [KYUUBI #2461] Use the original host argument
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2462:
URL: https://github.com/apache/incubator-kyuubi/pull/2462#issuecomment-1107837034
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2462?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 [#2462](https://codecov.io/gh/apache/incubator-kyuubi/pull/2462?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1a0a60e) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/3ecdd42286d2b47b3c2b7b090f4041e8c52e5e99?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3ecdd42) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #2462 +/- ##
=========================================
Coverage 63.18% 63.18%
Complexity 69 69
=========================================
Files 366 366
Lines 17442 17443 +1
Branches 2342 2342
=========================================
+ Hits 11020 11022 +2
+ Misses 5396 5395 -1
Partials 1026 1026
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/2462?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/ctl/ServiceControlCliArguments.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2462/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-a3l1dWJpLWN0bC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jdGwvU2VydmljZUNvbnRyb2xDbGlBcmd1bWVudHMuc2NhbGE=) | `79.54% <100.00%> (+0.11%)` | :arrow_up: |
| [.../org/apache/kyuubi/operation/KyuubiOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2462/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) | `66.66% <0.00%> (-1.34%)` | :arrow_down: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2462/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.44% <0.00%> (-0.12%)` | :arrow_down: |
| [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2462/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=) | `80.74% <0.00%> (+0.62%)` | :arrow_up: |
| [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2462/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) | `71.26% <0.00%> (+2.29%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2462?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/2462?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 [3ecdd42...1a0a60e](https://codecov.io/gh/apache/incubator-kyuubi/pull/2462?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] ulysses-you commented on pull request #2462: [KYUUBI #2461] Use the original host argument
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #2462:
URL: https://github.com/apache/incubator-kyuubi/pull/2462#issuecomment-1107989359
shall we just pass the input string which respect user specified ? It seems it's unnecessary to parse it to hostname
--
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