This is an automated email from the ASF dual-hosted git repository.
chengpan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kyuubi.git
The following commit(s) were added to refs/heads/master by this push:
new a4c90ccac [KYUUBI #5921] `EngineRef#deregister` should return the value
a4c90ccac is described below
commit a4c90ccace578d90a403e4ae49c26ef4cd3af6f2
Author: sychen <[email protected]>
AuthorDate: Fri Dec 29 19:09:41 2023 +0800
[KYUUBI #5921] `EngineRef#deregister` should return the value
# :mag: Description
## Issue References ๐
This pull request fixes #
## Describe Your Solution ๐ง
1. Let `org.apache.kyuubi.engine.EngineRef#deregister` have a return value
so that you can determine whether `deregister` is successful.
2. Fix a flaky test, add `eventually`, because there is a delay in delete,
causing UT to fail.
```
- deregister engine with existing host port *** FAILED ***
Some(("localhost", 42445)) was not empty (EngineRefTests.scala:362)
```
https://github.com/apache/kyuubi/actions/runs/7320908364/job/19940635471
## Types of changes :bookmark:
- [x] 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 :coffin:
flaky test
https://github.com/apache/kyuubi/actions/runs/7320908364/job/19940635471
#### Behavior With This Pull Request :tada:
GA
#### Related Unit Tests
EngineRefTests.scala
"deregister engine with existing host port"
---
# Checklists
## ๐ Author Self Checklist
- [ ] My code follows the [style
guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html)
of this project
- [ ] I have performed a self-review
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature
works
- [ ] New and existing unit tests pass locally with my changes
- [ ] This patch was not authored or co-authored using [Generative
Tooling](https://www.apache.org/legal/generative-tooling.html)
## ๐ Committer Pre-Merge Checklist
- [x] Pull request title is okay.
- [x] No license issues.
- [x] Milestone correctly set?
- [x] Test coverage is ok
- [x] Assignees are selected.
- [x] Minimum number of approvals
- [x] No changes are requested
**Be nice. Be informative.**
Closes #5921 from cxzl25/engine_deregister.
Closes #5921
1384a4312 [sychen] reorder and doc
c61bfbef6 [sychen] deregister
Authored-by: sychen <[email protected]>
Signed-off-by: Cheng Pan <[email protected]>
---
.../main/scala/org/apache/kyuubi/engine/EngineRef.scala | 17 +++++++++++++----
.../scala/org/apache/kyuubi/engine/EngineRefTests.scala | 9 ++++++---
2 files changed, 19 insertions(+), 7 deletions(-)
diff --git
a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
index bd1a4b640..ae32b04f0 100644
--- a/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
+++ b/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
@@ -297,6 +297,7 @@ private[kyuubi] class EngineRef(
*
* @param discoveryClient the zookeeper client to get or create engine
instance
* @param extraEngineLog the launch engine operation log, used to inject
engine log into it
+ * @return engine host and port
*/
def getOrCreate(
discoveryClient: DiscoveryClient,
@@ -312,19 +313,27 @@ private[kyuubi] class EngineRef(
*
* @param discoveryClient the zookeeper client to get or create engine
instance
* @param hostPort the existing engine host and port
+ * @return deregister result and message
*/
- def deregister(discoveryClient: DiscoveryClient, hostPort: (String, Int)):
Unit =
+ def deregister(discoveryClient: DiscoveryClient, hostPort: (String, Int)):
(Boolean, String) =
tryWithLock(discoveryClient) {
// refer the DiscoveryClient::getServerHost implementation
discoveryClient.getServiceNodesInfo(engineSpace, Some(1), silent = true)
match {
case Seq(sn) =>
if ((sn.host, sn.port) == hostPort) {
- info(s"Deleting engine node:$sn")
+ val msg = s"Deleting engine node:$sn"
+ info(msg)
discoveryClient.delete(s"$engineSpace/${sn.nodeName}")
+ (true, msg)
} else {
- warn(s"Engine node:$sn is not matched with host&port[$hostPort]")
+ val msg = s"Engine node:$sn is not matched with
host&port[$hostPort]"
+ warn(msg)
+ (false, msg)
}
- case _ => warn(s"No engine node found in $engineSpace")
+ case _ =>
+ val msg = s"No engine node found in $engineSpace"
+ warn(msg)
+ (false, msg)
}
}
diff --git
a/kyuubi-server/src/test/scala/org/apache/kyuubi/engine/EngineRefTests.scala
b/kyuubi-server/src/test/scala/org/apache/kyuubi/engine/EngineRefTests.scala
index af927960d..4341c5415 100644
--- a/kyuubi-server/src/test/scala/org/apache/kyuubi/engine/EngineRefTests.scala
+++ b/kyuubi-server/src/test/scala/org/apache/kyuubi/engine/EngineRefTests.scala
@@ -22,6 +22,7 @@ import java.util.concurrent.Executors
import scala.collection.JavaConverters._
+import org.scalatest.concurrent.PatienceConfiguration.Timeout
import org.scalatest.time.SpanSugar.convertIntToGrainOfTime
import org.apache.kyuubi.{KYUUBI_VERSION, Utils}
@@ -356,10 +357,12 @@ trait EngineRefTests extends KyuubiFunSuite {
DiscoveryClientProvider.withDiscoveryClient(conf) { client =>
val hp = engine.getOrCreate(client)
assert(client.getServerHost(engine.engineSpace) == Option(hp))
- engine.deregister(client, ("non_existing_host", 0))
+ assert(!engine.deregister(client, ("non_existing_host", 0))._1)
assert(client.getServerHost(engine.engineSpace) == Option(hp))
- engine.deregister(client, hp)
- assert(client.getServerHost(engine.engineSpace).isEmpty)
+ assert(engine.deregister(client, hp)._1)
+ eventually(Timeout(10.seconds)) {
+ assert(client.getServerHost(engine.engineSpace).isEmpty)
+ }
}
}
}
