[KYUUBI #2644] Add etcd discovery client for HA #2767

hddong · 2022-05-27T09:47:35Z

Why are the changes needed?

Add etcd discovery client for HA

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 locally before make a pull request

codecov-commenter · 2022-05-31T02:18:47Z

Codecov Report

Merging #2767 (6fa3757) into master (1bc3916) will increase coverage by 0.13%.
The diff coverage is 74.05%.

@@             Coverage Diff              @@
##             master    #2767      +/-   ##
============================================
+ Coverage     50.42%   50.55%   +0.13%     
  Complexity        6        6              
============================================
  Files           454      455       +1     
  Lines         25480    25631     +151     
  Branches       3576     3589      +13     
============================================
+ Hits          12848    12959     +111     
- Misses        11409    11436      +27     
- Partials       1223     1236      +13

Impacted Files	Coverage Δ
...he/kyuubi/ha/client/etcd/EtcdDiscoveryClient.scala	`72.41% <72.41%> (ø)`
...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala	`98.23% <85.71%> (-0.84%)`	⬇️
...ubi/ha/client/zookeeper/ZookeeperACLProvider.scala	`85.00% <100.00%> (ø)`
.../ha/client/zookeeper/ZookeeperClientProvider.scala	`71.01% <100.00%> (ø)`
...ha/client/zookeeper/ZookeeperDiscoveryClient.scala	`69.87% <100.00%> (ø)`
...ain/scala/org/apache/kyuubi/engine/EngineRef.scala	`75.23% <100.00%> (ø)`
.../org/apache/kyuubi/operation/KyuubiOperation.scala	`67.60% <0.00%> (-1.41%)`	⬇️
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala	`97.24% <0.00%> (-0.10%)`	⬇️
...a/org/apache/kyuubi/service/TFrontendService.scala	`91.44% <0.00%> (+0.29%)`	⬆️
...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala	`83.75% <0.00%> (+0.62%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bc3916...6fa3757. Read the comment docs.

hddong · 2022-05-31T05:57:14Z

cc @yaooqinn @pan3793

zwangsheng · 2022-05-31T06:58:49Z

kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/etcd/EtcdDiscoveryClient.scala

+    val ns = DiscoveryPaths.makePath(null, namespace)
+    create(ns, "PERSISTENT")
+
+    val session = conf.get(HA_ZK_ENGINE_REF_ID)


Maybe we should rename this configuration as it is used in both ETCD and ZK

Sorry for feedback late, done.

hddong · 2022-06-09T01:35:49Z

@yaooqinn @pan3793 : Please have a review when free.

pan3793 · 2022-06-12T18:10:07Z

dev/dependencyList

@@ -59,6 +68,7 @@ jakarta.validation-api/2.0.2//jakarta.validation-api-2.0.2.jar
 jakarta.ws.rs-api/2.1.6//jakarta.ws.rs-api-2.1.6.jar
 jakarta.xml.bind-api/2.3.2//jakarta.xml.bind-api-2.3.2.jar
 javassist/3.25.0-GA//javassist-3.25.0-GA.jar
+javax.annotation-api/1.3.2//javax.annotation-api-1.3.2.jar


We don't need this, jakarta.annotation-api is a drop-in replacement

pan3793 · 2022-06-12T18:10:48Z

dev/dependencyList

@@ -87,9 +101,15 @@ metrics-json/4.2.8//metrics-json-4.2.8.jar
 metrics-jvm/4.2.8//metrics-jvm-4.2.8.jar
 netty-all/4.1.73.Final//netty-all-4.1.73.Final.jar
 netty-buffer/4.1.73.Final//netty-buffer-4.1.73.Final.jar
+netty-codec-dns/4.1.74.Final//netty-codec-dns-4.1.74.Final.jar


gRPC uses a shaded netty, why do we need those netty stuff?

gRPC uses a shaded netty, why do we need those netty stuff?

These netty jars are a dependency of jetcd, had try to use netty-shaded, but got ClassNoDefException. Classes in netty-shaded have prefix io.grpc, jetcd can not recognized. Any suggestions?

etcd-io/jetcd#1068 seems going to provide an all-in-one client jar then we can get rid of those dependencies.

etcd-io/jetcd#1068 seems going to provide an all-in-one client jar then we can get rid of those dependencies.

Yep, we can provide an additional pr to change to shaded jar after it was merged and released

pan3793 · 2022-06-12T18:12:56Z

...ls/kyuubi-spark-sql-engine/src/main/resources/META-INF/services/io.grpc.LoadBalancerProvider

@@ -0,0 +1,3 @@
+io.grpc.internal.PickFirstLoadBalancerProvider
+io.grpc.util.SecretRoundRobinLoadBalancerProvider$Provider
+io.grpc.grpclb.GrpclbLoadBalancerProvider


What doesn't relocate them?

What doesn't relocate them?

Had do this, and got

Caused by: java.lang.IllegalArgumentException: cannot find a NameResolver for ip:///100.71.36.42:2389 at org.apache.kyuubi.shade.io.grpc.internal.ManagedChannelImpl.getNameResolver(ManagedChannelImpl.java:776) at org.apache.kyuubi.shade.io.grpc.internal.ManagedChannelImpl.getNameResolver(ManagedChannelImpl.java:785) at org.apache.kyuubi.shade.io.grpc.internal.ManagedChannelImpl.<init>(ManagedChannelImpl.java:665) at org.apache.kyuubi.shade.io.grpc.internal.ManagedChannelImplBuilder.build(ManagedChannelImplBuilder.java:630) at org.apache.kyuubi.shade.io.grpc.internal.AbstractManagedChannelImplBuilder.build(AbstractManagedChannelImplBuilder.java:297) at org.apache.kyuubi.shade.io.vertx.grpc.VertxChannelBuilder.build(VertxChannelBuilder.java:302) at io.etcd.jetcd.impl.ClientConnectionManager.getChannel(ClientConnectionManager.java:89)

Although we provide IPResolverProvider in nameresolverprovider, NameResolver can not load IPResolverProvider, we can relocate them after a solution is found.

pan3793 · 2022-06-12T18:16:13Z

kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClient.scala

@@ -43,7 +43,7 @@ trait DiscoveryClient extends Logging {
  /**
   * Get the stored data under path.
   */
-  def getData(path: String): Array[Byte]
+  def getData(path: String): String


We'd better avoid changing the API as much as possible.

pan3793 · 2022-06-12T18:17:10Z

kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/etcd/EtcdDiscoveryClient.scala

+    val pathPrefix = DiscoveryPaths.makePath(
+      namespace,
+      s"serviceUri=$instance;version=${version.getOrElse(KYUUBI_VERSION)};${session}sequence=")
+    val znodeData = instance


yaooqinn · 2022-06-13T07:18:48Z

...ls/kyuubi-spark-sql-engine/src/main/resources/META-INF/services/io.grpc.LoadBalancerProvider

@@ -0,0 +1,3 @@
+io.grpc.internal.PickFirstLoadBalancerProvider


license can be added here

license can be added here

Done

yaooqinn · 2022-06-13T07:18:58Z

...ls/kyuubi-spark-sql-engine/src/main/resources/META-INF/services/io.grpc.NameResolverProvider

@@ -0,0 +1,4 @@
+io.etcd.jetcd.resolver.DnsSrvResolverProvider


pom.xml

kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/etcd/EtcdUtils.scala

hddong · 2022-07-01T02:41:17Z

@yaooqinn @pan3793 @zwangsheng : Took some time to fix the tests, can review this again.

zwangsheng

LGTM

zwangsheng · 2022-07-05T10:01:19Z

kyuubi-ha/src/test/scala/org/apache/kyuubi/ha/client/etcd/EtcdDiscoveryClientSuite.scala

+
+  val conf: KyuubiConf = {
+    KyuubiConf()
+      .set(HA_CLIENT_CLASS, classOf[EtcdDiscoveryClient].getName)


How about we describe the option EtcdDiscoveryClient in the HA_CLIENT_CLASS docs, after all, ETCD is not mentioned in the docs at present.

yaooqinn · 2022-07-08T03:58:01Z

.rat-excludes

+**/io.grpc.LoadBalancerProvider
+**/io.grpc.NameResolverProvider


nit: remove them from rat-ex

yaooqinn · 2022-07-12T03:04:54Z

thanks, merged to master

hddong marked this pull request as draft May 27, 2022 09:47

github-actions bot added kind:build kind:documentation Documentation is a feature! module:ha module:server module:spark labels May 27, 2022

hddong self-assigned this May 27, 2022

github-actions bot added the kind:infra license, community building, project builds, asf infra related, etc. label May 30, 2022

hddong force-pushed the etcd-support branch from e860204 to 714096a Compare May 30, 2022 10:05

hddong force-pushed the etcd-support branch from 53a7a82 to 3210437 Compare May 31, 2022 03:19

hddong marked this pull request as ready for review May 31, 2022 05:56

hddong changed the title ~~[KYUUBI #2644][WIP] Add etcd discovery client for HA~~ [KYUUBI #2644] Add etcd discovery client for HA May 31, 2022

zwangsheng reviewed May 31, 2022

View reviewed changes

hddong force-pushed the etcd-support branch 2 times, most recently from afb980e to 0680687 Compare June 6, 2022 01:16

pan3793 reviewed Jun 12, 2022

View reviewed changes

yaooqinn reviewed Jun 13, 2022

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

yaooqinn reviewed Jun 13, 2022

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

yaooqinn reviewed Jun 13, 2022

View reviewed changes

kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/etcd/EtcdUtils.scala Outdated Show resolved Hide resolved

hddong force-pushed the etcd-support branch 2 times, most recently from 017cf11 to aef68b8 Compare June 22, 2022 07:53

hddong force-pushed the etcd-support branch 5 times, most recently from 297233a to ef0e51c Compare June 24, 2022 09:42

hddong force-pushed the etcd-support branch from d546fa1 to cdfd948 Compare June 29, 2022 08:28

zwangsheng approved these changes Jul 5, 2022

View reviewed changes

yaooqinn reviewed Jul 8, 2022

View reviewed changes

yaooqinn approved these changes Jul 8, 2022

View reviewed changes

hddong added 6 commits July 12, 2022 09:27

[KYUUBI apache#2644][WIP] Add etcd discovery client for HA

aa2b426

Rename HA_ZK_ENGINE_REF_ID to HA_ENGINE_REF_ID

9e81a6a

fix comments

28abd13

Fix tests

61c77d2

upgrade grpc-1.47.0

fc23a2b

Rebase and remove from rat-excludes

6fa3757

hddong force-pushed the etcd-support branch from de38ddb to 6fa3757 Compare July 12, 2022 01:55

yaooqinn added this to the v1.6.0 milestone Jul 12, 2022

yaooqinn closed this in 32970ce Jul 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KYUUBI #2644] Add etcd discovery client for HA #2767

[KYUUBI #2644] Add etcd discovery client for HA #2767

hddong commented May 27, 2022 •

edited

Loading

codecov-commenter commented May 31, 2022 •

edited

Loading

hddong commented May 31, 2022

zwangsheng May 31, 2022

hddong Jun 2, 2022

hddong commented Jun 9, 2022

pan3793 Jun 12, 2022

hddong Jun 21, 2022

pan3793 Jun 12, 2022 •

edited

Loading

hddong Jun 21, 2022

pan3793 Jun 21, 2022

hddong Jun 30, 2022

pan3793 Jun 12, 2022

hddong Jun 21, 2022

pan3793 Jun 12, 2022

pan3793 Jun 12, 2022

hddong Jun 21, 2022

yaooqinn Jun 13, 2022

hddong Jun 21, 2022

yaooqinn Jun 13, 2022

hddong Jun 21, 2022

hddong commented Jul 1, 2022

zwangsheng left a comment

zwangsheng Jul 5, 2022

yaooqinn Jul 8, 2022

yaooqinn commented Jul 12, 2022

		@@ -0,0 +1,3 @@
		io.grpc.internal.PickFirstLoadBalancerProvider

		@@ -0,0 +1,4 @@
		io.etcd.jetcd.resolver.DnsSrvResolverProvider

		**/io.grpc.LoadBalancerProvider
		**/io.grpc.NameResolverProvider

[KYUUBI #2644] Add etcd discovery client for HA #2767

[KYUUBI #2644] Add etcd discovery client for HA #2767

Conversation

hddong commented May 27, 2022 • edited Loading

Why are the changes needed?

How was this patch tested?

codecov-commenter commented May 31, 2022 • edited Loading

Codecov Report

hddong commented May 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hddong commented Jun 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan3793 Jun 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hddong commented Jul 1, 2022

zwangsheng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaooqinn commented Jul 12, 2022

hddong commented May 27, 2022 •

edited

Loading

codecov-commenter commented May 31, 2022 •

edited

Loading

pan3793 Jun 12, 2022 •

edited

Loading