pjfanning commented on code in PR #3699:
URL: https://github.com/apache/pekko/pull/3699#discussion_r4367733151
##########
cluster-tools/src/test/scala/org/apache/pekko/cluster/singleton/ClusterSingletonLeaseSpec.scala:
##########
@@ -217,6 +219,28 @@ class ClusterSingletonLeaseSpec extends
PekkoSpec(ConfigFactory.parseString("""
}
}
+ "log error when lease release fails leaving oldest" in {
+ val lifecycleProbe = TestProbe()
+ val settings = nextSettings()
+ system.actorOf(
+ ClusterSingletonManager.props(Props(new
ImportantSingleton(lifecycleProbe.ref)), PoisonPill, settings),
+ settings.singletonName)
+ val testLease = awaitAssert {
+ testLeaseExt.getTestLease(leaseNameFor(settings))
+ } // allow singleton manager to create the lease
+ testLease.probe.expectMsg(AcquireReq(leaseOwner))
+ testLease.initialPromise.complete(Success(true))
+ lifecycleProbe.expectMsg("preStart")
+ testLease.setNextReleaseResult(Future.failed(TestException("no release
for you")))
Review Comment:
This leaves the `TestLease` set to fail every subsequent `release()`, and
the manager keeps running: after the singleton terminates it re-acquires with
the already-completed `true` result and goes back to `Oldest`. Any later
release, e.g. during spec shutdown, would then log an unexpected
`TestException` error outside the `EventFilter`. Resetting it with
`testLease.setNextReleaseResult(Future.successful(true))`, or stopping the
manager at the end of the test, would keep it isolated.
##########
cluster-tools/src/main/scala/org/apache/pekko/cluster/singleton/ClusterSingletonManager.scala:
##########
@@ -1236,7 +1236,9 @@ class ClusterSingletonManager(singletonProps: Props,
terminationMessage: Any, se
lease.foreach { l =>
logInfo("Releasing lease as leaving Oldest")
import context.dispatcher
- pipe(l.release().map(ReleaseLeaseResult(_))).to(self)
+ pipe(l.release().map[Any](ReleaseLeaseResult(_)).recover {
Review Comment:
The release result still goes to `self`, so it can be lost on the
self-downed path. `Oldest` -> `Stopping` fires this release; if the singleton
terminates quickly, `Stopping` calls `stop()` before the lease backend's future
fails. `ReleaseLeaseFailure` then goes to dead letters, and because it extends
`DeadLetterSuppression` nothing is logged. That is the downing/hand-over case
where this diagnostic matters most. Could the failure be logged directly in the
future's `onComplete`, using a thread-safe `Logging(context.system, ...)`
adapter, rather than routed through the actor?
##########
cluster-tools/src/main/scala/org/apache/pekko/cluster/singleton/ClusterSingletonManager.scala:
##########
@@ -1236,7 +1236,9 @@ class ClusterSingletonManager(singletonProps: Props,
terminationMessage: Any, se
lease.foreach { l =>
logInfo("Releasing lease as leaving Oldest")
import context.dispatcher
- pipe(l.release().map(ReleaseLeaseResult(_))).to(self)
+ pipe(l.release().map[Any](ReleaseLeaseResult(_)).recover {
Review Comment:
`.recover` only handles a failed `Future`. A `Lease` implementation whose
`release()` throws synchronously (e.g. its client is not initialised) still
throws inside `onTransition` and restarts the manager mid-hand-over, without
this log line. Something like `Future.fromTry(Try(l.release())).flatten` would
route that case to `ReleaseLeaseFailure` too.
##########
cluster-tools/src/main/scala/org/apache/pekko/cluster/singleton/ClusterSingletonManager.scala:
##########
@@ -1236,7 +1236,9 @@ class ClusterSingletonManager(singletonProps: Props,
terminationMessage: Any, se
lease.foreach { l =>
logInfo("Releasing lease as leaving Oldest")
import context.dispatcher
- pipe(l.release().map(ReleaseLeaseResult(_))).to(self)
+ pipe(l.release().map[Any](ReleaseLeaseResult(_)).recover {
+ case t => ReleaseLeaseFailure(t)
Review Comment:
This release/map/recover/pipe chain is now duplicated word for word in the
`AcquiringLease` transition just above (lines 1227-1229). The two copies
drifting apart is what caused this bug, so a small `releaseLease()` helper next
to `tryAcquireLease` would keep them in sync, and the points in the other
comments would then only need fixing once.
Also, `tryAcquireLease` (line 848) uses `case NonFatal(t) =>` while both
release paths use a catch-all `case t =>`. It would be good to use `NonFatal`
consistently.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
