[KYUUBI #5952] Disconnect connections without running operations after engine maxlife time graceful period #6040

turboFei · 2024-02-04T01:51:18Z

🔍 Description

Issue References 🔗

We found that, some kyuubi connections(maybe managed by jdbc connection pool likes hikari) always keep alive, and the engine can not be terminated after exceeds the max life time.

So, In this pr, I introduce a graceful period after spark engine max life time, after the graceful period, the connections without running operations will be disconnected forcibly.

Close #5952

Describe Your Solution 🔧

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Types of changes 🔖

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklist 📝

This patch was not authored or co-authored using Generative Tooling

Be nice. Be informative.

ut ut revert save save debug Revert "debug" This reverts commit fe46552. retry

...s/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala

turboFei · 2024-02-04T05:17:45Z

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

@@ -68,7 +67,7 @@ abstract class ServiceDiscovery(
  def stopGracefully(isLost: Boolean = false): Unit = {
    while (fe.be.sessionManager.getOpenSessionCount > 0) {
      info(s"${fe.be.sessionManager.getOpenSessionCount} connection(s) are active, delay shutdown")
-      Thread.sleep(1000 * 60)
+      Thread.sleep(TimeUnit.SECONDS.toMillis(5))


short the sleep interval to pass UT

codecov-commenter · 2024-02-04T06:36:09Z

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (576379c) 61.06% compared to head (4329a85) 61.07%.
Report is 5 commits behind head on master.

❗ Current head 4329a85 differs from pull request most recent head 0b05ddc. Consider uploading reports for the commit 0b05ddc to get more accurate results

Files	Patch %	Lines
...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala	72.22%	2 Missing and 3 partials ⚠️
...org/apache/kyuubi/ha/client/ServiceDiscovery.scala	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #6040   +/-   ##
=========================================
  Coverage     61.06%   61.07%           
  Complexity       23       23           
=========================================
  Files           623      623           
  Lines         37160    37180   +20     
  Branches       5038     5038           
=========================================
+ Hits          22693    22708   +15     
- Misses        12012    12015    +3     
- Partials       2455     2457    +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

turboFei · 2024-02-04T18:11:03Z

...s/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala

+              // for ETCD, the de-registering process might be blocked, so deregister it async
+              info(s"Spark engine has been running for more than $maxLifetime ms," +
+                s" deregistering from engine discovery space.")
+              frontendServices.flatMap(_.discoveryService).foreach(_.stop())


09:46:29.180 vertx-blocked-thread-checker WARN BlockedThreadChecker: Thread Thread[vert.x-eventloop-thread-0,5,main] has been blocked for 19018 ms, time limit is 2000 ms io.vertx.core.VertxException: Thread blocked at java.lang.Thread.sleep(Native Method) at org.apache.kyuubi.ha.client.ServiceDiscovery.stopGracefully(ServiceDiscovery.scala:71) at org.apache.kyuubi.ha.client.etcd.EtcdDiscoveryClient$DeRegisterWatcher.$anonfun$onNext$2(EtcdDiscoveryClient.scala:384) at org.apache.kyuubi.ha.client.etcd.EtcdDiscoveryClient$DeRegisterWatcher.$anonfun$onNext$2$adapted(EtcdDiscoveryClient.scala:381) at org.apache.kyuubi.ha.client.etcd.EtcdDiscoveryClient$DeRegisterWatcher$$Lambda$2964/1803489267.apply(Unknown Source) at scala.collection.mutable.ResizableArray.foreach(ResizableArray.scala:62) at scala.collection.mutable.ResizableArray.foreach$(ResizableArray.scala:55) at scala.collection.mutable.ArrayBuffer.foreach(ArrayBuffer.scala:49) at org.apache.kyuubi.ha.client.etcd.EtcdDiscoveryClient$DeRegisterWatcher.onNext(EtcdDiscoveryClient.scala:381) at io.etcd.jetcd.impl.WatchImpl$WatcherImpl.onNext(WatchImpl.java:307) at io.etcd.jetcd.impl.WatchImpl$WatcherImpl$$Lambda$2892/1233911532.handle(Unknown Source) at io.vertx.grpc.stub.StreamObserverReadStream.onNext(StreamObserverReadStream.java:37) at io.grpc.stub.ClientCalls$StreamObserverToCallListenerAdapter.onMessage(ClientCalls.java:468) at io.grpc.internal.ClientCallImpl$ClientStreamListenerImpl$1MessagesAvailable.runInternal(ClientCallImpl.java:667) at io.grpc.internal.ClientCallImpl$ClientStreamListenerImpl$1MessagesAvailable.runInContext(ClientCallImpl.java:654) at io.grpc.internal.ContextRunnable.run(ContextRunnable.java:37) at io.grpc.internal.SerializingExecutor.run(SerializingExecutor.java:133) at io.vertx.grpc.VertxChannelBuilder.lambda$null$0(VertxChannelBuilder.java:305) at io.vertx.grpc.VertxChannelBuilder$$Lambda$2847/1899882869.handle(Unknown Source) at io.vertx.core.impl.ContextInternal.dispatch(ContextInternal.java:276)

Why do we need to deregister it async? The blocked vertx-blocked-thread-checker thread does not seem to be the checker thread.

@wForget This is the UT failure without async stop. https://github.com/apache/kyuubi/actions/runs/7771875352/job/21193688433

I see, discoveryService.stop() blocks the subsequent close session operation, causing the EngineServiceDiscovery.stopGracefully method to continue waiting.

I don't know. It doesn't happen every time.

Anyway, the current solution looks good to me.

the async deregister may introduce issues, say for some reason the deregistering takes 3s to complete and we stop the frontend/backend services, new incoming sessions will get errors

if only etcd has such an issue, let's add an internal configuration to switch the sync/async behavior. the Guava's SameThreadExecutorService may help

How about we execute stopGracefully asynchronously in DeRegisterWatcher#onNext?

turboFei · 2024-02-04T19:30:03Z

cc @bowenliang123 and @pan3793
It passed the UT eventually.

bowenliang123

LGTM.

pan3793

LGTM, pending CI

…s after engine maxlife time graceful period # 🔍 Description ## Issue References 🔗 We found that, some kyuubi connections(maybe managed by jdbc connection pool likes hikari) always keep alive, and the engine can not be terminated after exceeds the max life time. So, In this pr, I introduce a graceful period after spark engine max life time, after the graceful period, the connections without running operations will be disconnected forcibly. Close apache#5952 ## Describe Your Solution 🔧 Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklist 📝 - [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) **Be nice. Be informative.** Closes apache#6040 from turboFei/close_on_nooperation_rebase. Closes apache#5952 0b05ddc [Fei Wang] comments 4329a85 [Fei Wang] async stop b39c9b3 [Fei Wang] use short sleep 7c123f6 [Fei Wang] save 45ad348 [Fei Wang] check no running operation Authored-by: Fei Wang <fwang12@ebay.com> Signed-off-by: Fei Wang <fwang12@ebay.com> (cherry picked from commit 8c3f471)

… after engine maxlife time graceful period # 🔍 Description ## Issue References 🔗 We found that, some kyuubi connections(maybe managed by jdbc connection pool likes hikari) always keep alive, and the engine can not be terminated after exceeds the max life time. So, In this pr, I introduce a graceful period after spark engine max life time, after the graceful period, the connections without running operations will be disconnected forcibly. Close #5952 ## Describe Your Solution 🔧 Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklist 📝 - [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) **Be nice. Be informative.** Closes #6040 from turboFei/close_on_nooperation_rebase. Closes #5952 0b05ddc [Fei Wang] comments 4329a85 [Fei Wang] async stop b39c9b3 [Fei Wang] use short sleep 7c123f6 [Fei Wang] save 45ad348 [Fei Wang] check no running operation Authored-by: Fei Wang <fwang12ebay.com> Signed-off-by: Fei Wang <fwang12ebay.com> (cherry picked from commit 8c3f471) # 🔍 Description ## Issue References 🔗 This pull request fixes # ## Describe Your Solution 🔧 Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklist 📝 - [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) **Be nice. Be informative.** Closes #6054 from wForget/cherry_pick_6040_to_1_8. Closes #5952 0749d75 [Fei Wang] [KYUUBI #5952] Disconnect connections without running operations after engine maxlife time graceful period Authored-by: Fei Wang <fwang12@ebay.com> Signed-off-by: Cheng Pan <chengpan@apache.org>

…s after engine maxlife time graceful period # 🔍 Description ## Issue References 🔗 We found that, some kyuubi connections(maybe managed by jdbc connection pool likes hikari) always keep alive, and the engine can not be terminated after exceeds the max life time. So, In this pr, I introduce a graceful period after spark engine max life time, after the graceful period, the connections without running operations will be disconnected forcibly. Close apache#5952 ## Describe Your Solution 🔧 Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklist 📝 - [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) **Be nice. Be informative.** Closes apache#6040 from turboFei/close_on_nooperation_rebase. Closes apache#5952 0b05ddc [Fei Wang] comments 4329a85 [Fei Wang] async stop b39c9b3 [Fei Wang] use short sleep 7c123f6 [Fei Wang] save 45ad348 [Fei Wang] check no running operation Authored-by: Fei Wang <fwang12@ebay.com> Signed-off-by: Fei Wang <fwang12@ebay.com>

…ations after engine maxlife time graceful period # 🔍 Description ## Issue References 🔗 We found that, some kyuubi connections(maybe managed by jdbc connection pool likes hikari) always keep alive, and the engine can not be terminated after exceeds the max life time. So, In this pr, I introduce a graceful period after spark engine max life time, after the graceful period, the connections without running operations will be disconnected forcibly. Close apache#5952 ## Describe Your Solution 🔧 Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklist 📝 - [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) **Be nice. Be informative.** Closes apache#6040 from turboFei/close_on_nooperation_rebase. Closes apache#5952 0b05ddc [Fei Wang] comments 4329a85 [Fei Wang] async stop b39c9b3 [Fei Wang] use short sleep 7c123f6 [Fei Wang] save 45ad348 [Fei Wang] check no running operation Authored-by: Fei Wang <fwang12ebay.com> Signed-off-by: Fei Wang <fwang12ebay.com> (cherry picked from commit 8c3f471) # 🔍 Description ## Issue References 🔗 This pull request fixes # ## Describe Your Solution 🔧 Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklist 📝 - [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) **Be nice. Be informative.** Closes apache#6054 from wForget/cherry_pick_6040_to_1_8. Closes apache#5952 0749d75 [Fei Wang] [KYUUBI apache#5952] Disconnect connections without running operations after engine maxlife time graceful period Authored-by: Fei Wang <fwang12@ebay.com> Signed-off-by: Cheng Pan <chengpan@apache.org>

github-actions bot added kind:documentation Documentation is a feature! module:spark module:common labels Feb 4, 2024

check no running operation

45ad348

ut ut revert save save debug Revert "debug" This reverts commit fe46552. retry

turboFei force-pushed the close_on_nooperation_rebase branch from 739ccca to 45ad348 Compare February 4, 2024 01:52

save

7c123f6

github-actions bot added the module:ha label Feb 4, 2024

turboFei commented Feb 4, 2024

View reviewed changes

...s/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala Outdated Show resolved Hide resolved

turboFei commented Feb 4, 2024

View reviewed changes

use short sleep

b39c9b3

turboFei force-pushed the close_on_nooperation_rebase branch from 4fc4d71 to b39c9b3 Compare February 4, 2024 05:18

turboFei self-assigned this Feb 4, 2024

async stop

4329a85

turboFei force-pushed the close_on_nooperation_rebase branch from 992aefd to 4329a85 Compare February 4, 2024 18:05

turboFei commented Feb 4, 2024

View reviewed changes

bowenliang123 approved these changes Feb 5, 2024

View reviewed changes

wForget approved these changes Feb 6, 2024

View reviewed changes

wForget requested a review from pan3793 February 6, 2024 03:04

comments

0b05ddc

turboFei force-pushed the close_on_nooperation_rebase branch from dd34342 to 0b05ddc Compare February 6, 2024 06:52

pan3793 approved these changes Feb 6, 2024

View reviewed changes

turboFei closed this in 8c3f471 Feb 7, 2024

wForget added this to the v1.8.1 milestone Feb 8, 2024

wForget mentioned this pull request Feb 8, 2024

[KYUUBI #5952][1.8] Disconnect connections without running operations after engine maxlife time graceful period #6054

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KYUUBI #5952] Disconnect connections without running operations after engine maxlife time graceful period #6040

[KYUUBI #5952] Disconnect connections without running operations after engine maxlife time graceful period #6040

turboFei commented Feb 4, 2024 •

edited

Loading

turboFei Feb 4, 2024

codecov-commenter commented Feb 4, 2024 •

edited

Loading

turboFei Feb 4, 2024

wForget Feb 5, 2024

turboFei Feb 5, 2024

turboFei Feb 5, 2024

wForget Feb 6, 2024

turboFei Feb 6, 2024

wForget Feb 6, 2024

pan3793 Feb 6, 2024

pan3793 Feb 6, 2024 •

edited

Loading

wForget Feb 6, 2024

turboFei commented Feb 4, 2024

bowenliang123 left a comment

pan3793 left a comment

[KYUUBI #5952] Disconnect connections without running operations after engine maxlife time graceful period #6040

[KYUUBI #5952] Disconnect connections without running operations after engine maxlife time graceful period #6040

Conversation

turboFei commented Feb 4, 2024 • edited Loading

🔍 Description

Issue References 🔗

Describe Your Solution 🔧

Types of changes 🔖

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklist 📝

Choose a reason for hiding this comment

codecov-commenter commented Feb 4, 2024 • edited Loading

Codecov Report

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

pan3793 Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turboFei commented Feb 4, 2024

bowenliang123 left a comment

Choose a reason for hiding this comment

pan3793 left a comment

Choose a reason for hiding this comment

turboFei commented Feb 4, 2024 •

edited

Loading

codecov-commenter commented Feb 4, 2024 •

edited

Loading

pan3793 Feb 6, 2024 •

edited

Loading