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/05/09 06:38:05 UTC
[GitHub] [incubator-kyuubi] hiboyang opened a new pull request, #2601: Add a config to support different service discovery client class implementation
hiboyang opened a new pull request, #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601
### _Why are the changes needed?_
We want to implement different service discovery client, thus add a config to support different service discovery client class implementation.
### _How was this patch tested?_
- [ x] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [ x] [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] pan3793 commented on pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#issuecomment-1120705318
Thanks for contribution. May I know which discovery service would you like to support?
--
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] hiboyang commented on pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hiboyang commented on PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#issuecomment-1120723802
> Thanks for contribution. May I know which discovery service would you like to support?
I am thinking to add some discovery service to make it easy running in Kubernetes. Not finalize my thought yet, but would like to make this configurable first, then I could play with different implementation.
--
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] hddong closed pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hddong closed pull request #2601: Add a config to support different service discovery client class implementation
URL: https://github.com/apache/incubator-kyuubi/pull/2601
--
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] hiboyang commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hiboyang commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r868829665
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -186,6 +186,26 @@ object KyuubiConf {
ConfigBuilder(key).onCreate(register)
}
+ /**
+ * Create an object instance with given [[KyuubiConf]].
+ * @param clazz object class
+ * @param conf configuration ([[KyuubiConf]])
+ * @tparam T object instance type to create
+ * @return
+ */
+ def createInstance[T](clazz: Class[_], conf: KyuubiConf): T = {
Review Comment:
Sounds good
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1468,4 +1488,11 @@ object KyuubiConf {
.version("1.6.0")
.booleanConf
.createWithDefault(false)
+
+ val DISCOVERY_CLIENT_CLASS: ConfigEntry[String] =
+ buildConf("kyuubi.service.discovery.client.class")
+ .doc("Class name for service discovery client.")
+ .version("1.6.0")
Review Comment:
yes
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1468,4 +1488,11 @@ object KyuubiConf {
.version("1.6.0")
.booleanConf
.createWithDefault(false)
+
+ val DISCOVERY_CLIENT_CLASS: ConfigEntry[String] =
Review Comment:
Yes
--
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] hiboyang commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hiboyang commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r869859298
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClientProvider.scala:
##########
@@ -42,4 +43,19 @@ object DiscoveryClientProvider extends Logging {
}
}
+ def createDiscoveryClient(conf: KyuubiConf): DiscoveryClient = {
+ val classLoader = Thread.currentThread.getContextClassLoader
+ val className = conf.get(HighAvailabilityConf.DISCOVERY_CLIENT_CLASS)
+ if (className.isEmpty) {
Review Comment:
This check still valid in case people manually set the configure value to empty string?
--
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] yaooqinn commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r867703988
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -186,6 +186,26 @@ object KyuubiConf {
ConfigBuilder(key).onCreate(register)
}
+ /**
+ * Create an object instance with given [[KyuubiConf]].
+ * @param clazz object class
+ * @param conf configuration ([[KyuubiConf]])
+ * @tparam T object instance type to create
+ * @return
+ */
+ def createInstance[T](clazz: Class[_], conf: KyuubiConf): T = {
Review Comment:
move to org.apache.kyuubi.Utils？
--
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 #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r867676029
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClientProvider.scala:
##########
@@ -42,4 +41,15 @@ object DiscoveryClientProvider extends Logging {
}
}
+ def createDiscoveryClient(conf: KyuubiConf): DiscoveryClient = {
+ val className = conf.get(KyuubiConf.DISCOVERY_CLIENT_CLASS)
+ val clazz =
+ try {
+ Class.forName(className)
+ } catch {
+ case ex: Throwable => throw new KyuubiException(s"Class not found $className", ex)
+ }
+ val constructor = clazz.getConstructor(conf.getClass)
+ constructor.newInstance(conf).asInstanceOf[DiscoveryClient]
Review Comment:
Another check is required here, please refer `AuthenticationProviderFactory.getAuthenticationProviderForServer`
--
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] yaooqinn commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r869890210
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClientProvider.scala:
##########
@@ -42,4 +43,19 @@ object DiscoveryClientProvider extends Logging {
}
}
+ def createDiscoveryClient(conf: KyuubiConf): DiscoveryClient = {
+ val classLoader = Thread.currentThread.getContextClassLoader
+ val className = conf.get(HighAvailabilityConf.DISCOVERY_CLIENT_CLASS)
+ if (className.isEmpty) {
Review Comment:
I guess it is never reached
--
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] hddong commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hddong commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r868842380
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala:
##########
@@ -158,4 +158,12 @@ object HighAvailabilityConf {
.version("1.5.0")
.stringConf
.createOptional
+
+ val DISCOVERY_CLIENT_CLASS: ConfigEntry[String] =
+ buildConf("kyuubi.service.discovery.client.class")
+ .doc("Class name for service discovery client.")
+ .version("1.6.0")
+ .stringConf
+ .checkValue(!_.isEmpty, "must not be empty")
Review Comment:
Shall we use `nonEmpty` 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] hiboyang commented on pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hiboyang commented on PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#issuecomment-1123225239
> please revert the unrelated changes
oops, my code is messed up, let me see whether I could clean it up, or I shall close this PR.
--
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] yaooqinn commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r868871003
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala:
##########
@@ -158,4 +158,12 @@ object HighAvailabilityConf {
.version("1.5.0")
.stringConf
.createOptional
+
+ val DISCOVERY_CLIENT_CLASS: ConfigEntry[String] =
+ buildConf("kyuubi.service.discovery.client.class")
+ .doc("Class name for service discovery client.")
+ .version("1.6.0")
+ .stringConf
+ .checkValue(_.nonEmpty, "must not be empty")
Review Comment:
use classOf[DiscoveryClient].isAssignableFrom(cls)
--
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] hiboyang commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hiboyang commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r869898418
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClientProvider.scala:
##########
@@ -42,4 +43,19 @@ object DiscoveryClientProvider extends Logging {
}
}
+ def createDiscoveryClient(conf: KyuubiConf): DiscoveryClient = {
+ val classLoader = Thread.currentThread.getContextClassLoader
+ val className = conf.get(HighAvailabilityConf.DISCOVERY_CLIENT_CLASS)
+ if (className.isEmpty) {
Review Comment:
Maybe, "className.isEmpty" is a simple check, should not hurt to keep it, in case other parts of the code have issue and do not deal with that config value properly.
--
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] hiboyang commented on pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hiboyang commented on PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#issuecomment-1123231895
> > please revert the unrelated changes
>
> oops, my code is messed up, let me see whether I could clean it up, or I shall close this PR.
reverted unrelated changes.
--
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] hiboyang commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hiboyang commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r868846025
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala:
##########
@@ -158,4 +158,12 @@ object HighAvailabilityConf {
.version("1.5.0")
.stringConf
.createOptional
+
+ val DISCOVERY_CLIENT_CLASS: ConfigEntry[String] =
+ buildConf("kyuubi.service.discovery.client.class")
+ .doc("Class name for service discovery client.")
+ .version("1.6.0")
+ .stringConf
+ .checkValue(!_.isEmpty, "must not be empty")
Review Comment:
np, changed to nonEmpty
--
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] hiboyang commented on pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hiboyang commented on PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#issuecomment-1121956120
@yaooqinn @pan3793 thanks for the comments, I updated the code, would you help to check again?
--
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] hiboyang commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hiboyang commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r867696750
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClientProvider.scala:
##########
@@ -42,4 +41,15 @@ object DiscoveryClientProvider extends Logging {
}
}
+ def createDiscoveryClient(conf: KyuubiConf): DiscoveryClient = {
+ val className = conf.get(KyuubiConf.DISCOVERY_CLIENT_CLASS)
+ val clazz =
+ try {
+ Class.forName(className)
+ } catch {
+ case ex: Throwable => throw new KyuubiException(s"Class not found $className", ex)
+ }
+ val constructor = clazz.getConstructor(conf.getClass)
+ constructor.newInstance(conf).asInstanceOf[DiscoveryClient]
Review Comment:
Thanks for the suggestion! Updated the PR.
--
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] hddong commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hddong commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r870893555
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala:
##########
@@ -158,4 +158,12 @@ object HighAvailabilityConf {
.version("1.5.0")
.stringConf
.createOptional
+
+ val DISCOVERY_CLIENT_CLASS: ConfigEntry[String] =
+ buildConf("kyuubi.service.discovery.client.class")
Review Comment:
Shall we change to `kyuubi.ha.discovery.client.class`, the key should put under [`HA`](https://github.com/apache/incubator-kyuubi/blob/master/docs/deployment/settings.md#ha) in settings.md .
--
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] yaooqinn commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r869904413
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClientProvider.scala:
##########
@@ -42,4 +43,19 @@ object DiscoveryClientProvider extends Logging {
}
}
+ def createDiscoveryClient(conf: KyuubiConf): DiscoveryClient = {
+ val classLoader = Thread.currentThread.getContextClassLoader
+ val className = conf.get(HighAvailabilityConf.DISCOVERY_CLIENT_CLASS)
+ if (className.isEmpty) {
Review Comment:
ok
--
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] yaooqinn commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r867705002
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1468,4 +1488,11 @@ object KyuubiConf {
.version("1.6.0")
.booleanConf
.createWithDefault(false)
+
+ val DISCOVERY_CLIENT_CLASS: ConfigEntry[String] =
+ buildConf("kyuubi.service.discovery.client.class")
+ .doc("Class name for service discovery client.")
+ .version("1.6.0")
Review Comment:
call checkValue 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] yaooqinn commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r867700123
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1468,4 +1488,11 @@ object KyuubiConf {
.version("1.6.0")
.booleanConf
.createWithDefault(false)
+
+ val DISCOVERY_CLIENT_CLASS: ConfigEntry[String] =
Review Comment:
move to HighAvailabilityConf？
--
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] yaooqinn commented on pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#issuecomment-1123223760
please revert the unrelated changes
--
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] hddong commented on pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hddong commented on PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#issuecomment-1123437073
@hiboyang : The tests failed, please run `KYUUBI_UPDATE=1 build/mvn clean install -Pflink-provided,spark-provided,hive-provided -DwildcardSuites=org.apache.kyuubi.config.AllKyuubiConfiguration` to update `settings.md` file.
--
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] yaooqinn commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r868872063
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClientProvider.scala:
##########
@@ -42,4 +43,19 @@ object DiscoveryClientProvider extends Logging {
}
}
+ def createDiscoveryClient(conf: KyuubiConf): DiscoveryClient = {
+ val classLoader = Thread.currentThread.getContextClassLoader
+ val className = conf.get(HighAvailabilityConf.DISCOVERY_CLIENT_CLASS)
+ if (className.isEmpty) {
Review Comment:
no need to check again as a result of https://github.com/apache/incubator-kyuubi/pull/2601/files#diff-884de93f6e9662de661d5ac820b28123d405bdf473912fc015cb1f84b70e8550R167
--
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] hiboyang commented on a diff in pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hiboyang commented on code in PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#discussion_r869858939
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala:
##########
@@ -158,4 +158,12 @@ object HighAvailabilityConf {
.version("1.5.0")
.stringConf
.createOptional
+
+ val DISCOVERY_CLIENT_CLASS: ConfigEntry[String] =
+ buildConf("kyuubi.service.discovery.client.class")
+ .doc("Class name for service discovery client.")
+ .version("1.6.0")
+ .stringConf
+ .checkValue(_.nonEmpty, "must not be empty")
Review Comment:
Here it is a string config, better avoid loading the class and check isAssignableFrom
--
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] hddong commented on pull request #2601: Add a config to support different service discovery client class implementation
Posted by GitBox <gi...@apache.org>.
hddong commented on PR #2601:
URL: https://github.com/apache/incubator-kyuubi/pull/2601#issuecomment-1124724025
@hiboyang : 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