Skip to content

Commit

Permalink
[KYUUBI #5921] EngineRef#deregister should return the value
Browse files Browse the repository at this point in the history
# 🔍 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 🔖
- [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 ⚰️
flaky test

https://github.com/apache/kyuubi/actions/runs/7320908364/job/19940635471

#### Behavior With This Pull Request 🎉
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

1384a43 [sychen] reorder and doc
c61bfbe [sychen] deregister

Authored-by: sychen <[email protected]>
Signed-off-by: Cheng Pan <[email protected]>
  • Loading branch information
cxzl25 authored and pan3793 committed Dec 29, 2023
1 parent 679aca5 commit a4c90cc
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -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)
}
}
}
}

0 comments on commit a4c90cc

Please sign in to comment.