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/02/03 08:59:50 UTC
[GitHub] [kyuubi] turboFei opened a new pull request, #4242: Support thrift binary non-ssl and ssl connections at the same time
turboFei opened a new pull request, #4242:
URL: https://github.com/apache/kyuubi/pull/4242
<!--
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 pull request #4242: Support thrift binary non-ssl and ssl connections at the same time
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #4242:
URL: https://github.com/apache/kyuubi/pull/4242#issuecomment-1415515869
how do you think about? @yaooqinn @pan3793
I think it should be common case if they want to make SSL migration.
--
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 pull request #4242: Support thrift binary non-ssl and ssl connections at the same time
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #4242:
URL: https://github.com/apache/kyuubi/pull/4242#issuecomment-1415528579
I think it's a common case, many services support to listen plain/ssl port at the same time.
--
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 #4242: Support thrift binary non-ssl and ssl connections at the same time
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #4242:
URL: https://github.com/apache/kyuubi/pull/4242#discussion_r1096905924
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -371,7 +371,7 @@ object KyuubiConf {
object FrontendProtocols extends Enumeration {
type FrontendProtocol = Value
- val THRIFT_BINARY, THRIFT_HTTP, REST, MYSQL, TRINO = Value
+ val THRIFT_BINARY, THRIFT_BINARY_SSL, THRIFT_HTTP, REST, MYSQL, TRINO = Value
Review Comment:
THRIFT_BINARY, THRIFT_BINARY_SSL seems not discrete mathematically
--
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 closed pull request #4242: Support thrift binary non-ssl and ssl connections at the same time
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 closed pull request #4242: Support thrift binary non-ssl and ssl connections at the same time
URL: https://github.com/apache/kyuubi/pull/4242
--
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 #4242: Support thrift binary non-ssl and ssl connections at the same time
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4242:
URL: https://github.com/apache/kyuubi/pull/4242#discussion_r1098250997
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -371,7 +371,7 @@ object KyuubiConf {
object FrontendProtocols extends Enumeration {
type FrontendProtocol = Value
- val THRIFT_BINARY, THRIFT_HTTP, REST, MYSQL, TRINO = Value
+ val THRIFT_BINARY, THRIFT_BINARY_SSL, THRIFT_HTTP, REST, MYSQL, TRINO = Value
Review Comment:
I think the idea here is:
1. THRIFT_BINARY, THRIFT_BINARY_SSL should be like HTTP, HTTPS eventually, there is discrete and can be enabled at the same time.
2. currently we keep SSL option of THRIFT_BINARY for compatibility, the deprecating message has been added and this feature will be removed in the 2.0
--
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 #4242: Support thrift binary non-ssl and ssl connections at the same time
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4242:
URL: https://github.com/apache/kyuubi/pull/4242#discussion_r1095595932
##########
docs/deployment/settings.md:
##########
@@ -311,6 +311,7 @@ You can configure the Kyuubi properties in `$KYUUBI_HOME/conf/kyuubi-defaults.co
| kyuubi.frontend.thrift.backoff.slot.length | PT0.1S | Time to back off during login to the thrift frontend service. | duration | 1.4.0 |
| kyuubi.frontend.thrift.binary.bind.host | <undefined> | Hostname or IP of the machine on which to run the thrift frontend service via the binary protocol. | string | 1.4.0 |
| kyuubi.frontend.thrift.binary.bind.port | 10009 | Port of the machine on which to run the thrift frontend service via the binary protocol. | int | 1.4.0 |
+| kyuubi.frontend.thrift.binary.ssl.bind.port | 10009 | Port of the machine on which to run the thrift frontend service via the binary protocol with SSL enabled. | int | 1.7.0 |
Review Comment:
we should give it a different value
--
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 pull request #4242: Support thrift binary non-ssl and ssl connections at the same time
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #4242:
URL: https://github.com/apache/kyuubi/pull/4242#issuecomment-1418428801
A bit weird to me for the naming part
--
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 #4242: Support thrift binary non-ssl and ssl connections at the same time
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei closed pull request #4242: Support thrift binary non-ssl and ssl connections at the same time
URL: https://github.com/apache/kyuubi/pull/4242
--
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 pull request #4242: Support thrift binary non-ssl and ssl connections at the same time
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #4242:
URL: https://github.com/apache/kyuubi/pull/4242#issuecomment-1418434287
This does not support HA and introduces bugs when the SSL on/off together w/ the same namespace.
--
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 #4242: Support thrift binary non-ssl and ssl connections at the same time
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #4242:
URL: https://github.com/apache/kyuubi/pull/4242#discussion_r1096905038
##########
docs/deployment/settings.md:
##########
@@ -311,6 +311,7 @@ You can configure the Kyuubi properties in `$KYUUBI_HOME/conf/kyuubi-defaults.co
| kyuubi.frontend.thrift.backoff.slot.length | PT0.1S | Time to back off during login to the thrift frontend service. | duration | 1.4.0 |
| kyuubi.frontend.thrift.binary.bind.host | <undefined> | Hostname or IP of the machine on which to run the thrift frontend service via the binary protocol. | string | 1.4.0 |
| kyuubi.frontend.thrift.binary.bind.port | 10009 | Port of the machine on which to run the thrift frontend service via the binary protocol. | int | 1.4.0 |
+| kyuubi.frontend.thrift.binary.ssl.bind.port | 10011 | Port of the machine on which to run the thrift frontend service via the binary protocol with SSL enabled. | int | 1.7.0 |
Review Comment:
kyuubi.frontend.thrift.binary.bind.port also can be used for ssl, how do we suppose users to understand?
--
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 #4242: Support thrift binary non-ssl and ssl connections at the same time
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4242:
URL: https://github.com/apache/kyuubi/pull/4242#discussion_r1098250997
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -371,7 +371,7 @@ object KyuubiConf {
object FrontendProtocols extends Enumeration {
type FrontendProtocol = Value
- val THRIFT_BINARY, THRIFT_HTTP, REST, MYSQL, TRINO = Value
+ val THRIFT_BINARY, THRIFT_BINARY_SSL, THRIFT_HTTP, REST, MYSQL, TRINO = Value
Review Comment:
I think the idea here is:
1. THRIFT_BINARY, THRIFT_BINARY_SSL should be like HTTP, HTTPS eventually, there are discrete and can be enabled at the same time.
2. currently we keep SSL option of THRIFT_BINARY for compatibility, the deprecating message has been added and this feature will be removed in the 2.0
--
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