Skip to content

Commit 203286c

Browse files
github-actions[bot]aglinxinyuanclaude
authored
fix(amber, operator, v1.2): keep error information on three failure paths (#7809)
### What changes were proposed in this PR? Automated backport of #7804 to `release/v1.2`. Source: 226124b · [automation run](https://github.com/apache/texera/actions/runs/32411776055) ### Any related issues, documentation, discussions? Backport of #7804. Originally linked #7803. ### How was this PR tested? Release-branch CI runs on this branch once the conflicts are resolved and this PR is marked ready for review. ### Was this PR authored or co-authored using generative AI tooling? No. --------- Co-authored-by: Xinyuan Lin <xinyual3@uci.edu> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 41cc7c5 commit 203286c

6 files changed

Lines changed: 856 additions & 2 deletions

File tree

amber/src/main/scala/org/apache/texera/amber/engine/architecture/common/PekkoActorRefMappingService.scala

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,8 +112,13 @@ class PekkoActorRefMappingService(actorService: PekkoActorService) extends Amber
112112
queriedActorVirtualIdentities.add(id)
113113
} catch {
114114
case e: Throwable =>
115+
// Deliberately does not read `actorService.parent` again: that is the value whose
116+
// failure this handler exists to contain, so re-reading it to name the parent ref
117+
// makes a persistently unreachable parent throw straight out of the catch block.
118+
// The exception carries the detail (including which lookup failed and why).
115119
logger.warn(
116-
s"Failed to fetch actorRef for ${VirtualIdentityUtils.toShorterString(id)} parentRef = " + actorService.parent
120+
s"Failed to fetch actorRef for ${VirtualIdentityUtils.toShorterString(id)} from parent",
121+
e
117122
)
118123
}
119124
}

amber/src/main/scala/org/apache/texera/amber/engine/architecture/messaginglayer/InputManager.scala

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,8 @@ class InputManager(
105105
} catch {
106106
case e: Exception =>
107107
throw new RuntimeException(
108-
s"Error starting input port materialization reader thread: ${e.getMessage}"
108+
s"Error starting input port materialization reader thread: ${e.getMessage}",
109+
e
109110
)
110111
}
111112
})
Lines changed: 236 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,236 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.apache.texera.amber.engine.architecture.common
21+
22+
import org.apache.pekko.actor.{Actor, ActorContext, ActorRef, ActorSystem, Props}
23+
import org.apache.pekko.testkit.{TestActorRef, TestKit, TestProbe}
24+
import org.apache.texera.amber.core.virtualidentity.{ActorVirtualIdentity, ChannelIdentity}
25+
import org.apache.texera.amber.engine.architecture.common.WorkflowActor.{
26+
CreditRequest,
27+
GetActorRef,
28+
NetworkMessage,
29+
RegisterActorRef
30+
}
31+
import org.apache.texera.amber.engine.common.ambermessage.{DataFrame, WorkflowFIFOMessage}
32+
import org.apache.texera.amber.engine.common.virtualidentity.util.CONTROLLER
33+
import org.scalatest.BeforeAndAfterAll
34+
import org.scalatest.flatspec.AnyFlatSpecLike
35+
36+
import scala.concurrent.duration.DurationInt
37+
38+
/**
39+
* Unit tests for [[PekkoActorRefMappingService]].
40+
*
41+
* These tests use a minimal actor spawned by Pekko TestKit to supply the
42+
* live [[ActorContext]] that [[PekkoActorService]] eagerly dereferences.
43+
* Spawning it as a child of a [[TestProbe]] also makes the service's parent
44+
* lookup observable.
45+
*/
46+
class PekkoActorRefMappingServiceSpec
47+
extends TestKit(ActorSystem("PekkoActorRefMappingServiceSpec"))
48+
with AnyFlatSpecLike
49+
with BeforeAndAfterAll {
50+
51+
override def afterAll(): Unit = {
52+
TestKit.shutdownActorSystem(system)
53+
}
54+
55+
private val upstreamId = ActorVirtualIdentity("upstream")
56+
private val workerId = ActorVirtualIdentity("mapping-service-worker")
57+
private val contextCounter = new java.util.concurrent.atomic.AtomicInteger(0)
58+
59+
private def channelTo(destination: ActorVirtualIdentity): ChannelIdentity =
60+
ChannelIdentity(
61+
fromWorkerId = upstreamId,
62+
toWorkerId = destination,
63+
isControl = true
64+
)
65+
66+
private def networkMessageTo(
67+
destination: ActorVirtualIdentity,
68+
messageId: Long,
69+
sequenceNumber: Long
70+
): NetworkMessage =
71+
NetworkMessage(
72+
messageId,
73+
WorkflowFIFOMessage(channelTo(destination), sequenceNumber, DataFrame(Array.empty))
74+
)
75+
76+
private def newContext(parent: TestProbe): ActorContext =
77+
TestActorRef[ActorRefMappingServiceContextHolder](
78+
Props(new ActorRefMappingServiceContextHolder),
79+
parent.ref,
80+
s"actor-ref-mapping-context-${contextCounter.incrementAndGet()}"
81+
).underlyingActor.context
82+
83+
private def newActorService(
84+
id: ActorVirtualIdentity,
85+
parent: TestProbe
86+
): PekkoActorService = new PekkoActorService(id, newContext(parent))
87+
88+
"askForCredit" should "forward a request only when the destination ref is known" in {
89+
val parent = TestProbe()
90+
val destination = TestProbe()
91+
val unknownDestination = ActorVirtualIdentity("unknown-credit-destination")
92+
val knownDestination = ActorVirtualIdentity("known-credit-destination")
93+
val service = new PekkoActorRefMappingService(newActorService(workerId, parent))
94+
95+
service.askForCredit(channelTo(unknownDestination))
96+
parent.expectNoMessage(100.millis)
97+
98+
service.registerActorRef(knownDestination, destination.ref)
99+
val channel = channelTo(knownDestination)
100+
service.askForCredit(channel)
101+
102+
destination.expectMsg(CreditRequest(channel))
103+
}
104+
105+
"forwardToActor" should "stash unknown messages and ask the parent for their ref once" in {
106+
val parent = TestProbe()
107+
val destination = ActorVirtualIdentity("unknown-message-destination")
108+
val service = new PekkoActorRefMappingService(newActorService(workerId, parent))
109+
val first = networkMessageTo(destination, messageId = 1L, sequenceNumber = 0L)
110+
val second = networkMessageTo(destination, messageId = 2L, sequenceNumber = 1L)
111+
112+
service.forwardToActor(first)
113+
val lookup = parent.expectMsgType[GetActorRef]
114+
assert(lookup.id == destination)
115+
assert(lookup.replyTo == Set(service.self))
116+
117+
service.forwardToActor(second)
118+
parent.expectNoMessage(100.millis)
119+
120+
service.clearQueriedActorRefs()
121+
service.forwardToActor(networkMessageTo(destination, messageId = 3L, sequenceNumber = 2L))
122+
assert(parent.expectMsgType[GetActorRef].id == destination)
123+
}
124+
125+
"registerActorRef" should "drain stashed messages in FIFO order and notify controller waiters" in {
126+
val parent = TestProbe()
127+
val destination = ActorVirtualIdentity("controller-wait-destination")
128+
val waiterOne = TestProbe()
129+
val waiterTwo = TestProbe()
130+
val registered = TestProbe()
131+
val service = new PekkoActorRefMappingService(newActorService(CONTROLLER, parent))
132+
val first = networkMessageTo(destination, messageId = 10L, sequenceNumber = 0L)
133+
val second = networkMessageTo(destination, messageId = 11L, sequenceNumber = 1L)
134+
135+
service.forwardToActor(first)
136+
service.forwardToActor(second)
137+
service.retrieveActorRef(destination, Set(waiterOne.ref, waiterTwo.ref))
138+
139+
service.registerActorRef(destination, registered.ref)
140+
141+
registered.expectMsg(first)
142+
registered.expectMsg(second)
143+
waiterOne.expectMsg(RegisterActorRef(destination, registered.ref))
144+
waiterTwo.expectMsg(RegisterActorRef(destination, registered.ref))
145+
}
146+
147+
"retrieveActorRef and removeActorRef" should "reply for known refs and remove their reverse lookup" in {
148+
val parent = TestProbe()
149+
val destination = ActorVirtualIdentity("registered-destination")
150+
val registered = TestProbe()
151+
val waiter = TestProbe()
152+
val service = new PekkoActorRefMappingService(newActorService(workerId, parent))
153+
154+
service.registerActorRef(destination, registered.ref)
155+
service.retrieveActorRef(destination, Set(waiter.ref))
156+
157+
waiter.expectMsg(RegisterActorRef(destination, registered.ref))
158+
assert(service.hasActorRef(destination))
159+
assert(service.getActorRef(destination) == registered.ref)
160+
assert(service.findActorVirtualIdentity(registered.ref).contains(destination))
161+
162+
service.removeActorRef(destination)
163+
164+
assert(!service.hasActorRef(destination))
165+
assert(service.findActorVirtualIdentity(registered.ref).isEmpty)
166+
}
167+
168+
"retrieveActorRef" should "swallow a failed parent lookup and still ask again for the same id" in {
169+
val parent = TestProbe()
170+
val destination = ActorVirtualIdentity("unreachable-parent-destination")
171+
val waiter = TestProbe()
172+
// Pekko itself does not fail here -- `context.parent` reads a field and `!` never throws -- so
173+
// the failure is injected, for the first read only.
174+
val actorService = new FailingParentActorService(workerId, newContext(parent))
175+
val service = new PekkoActorRefMappingService(actorService)
176+
actorService.failuresLeft = 1
177+
178+
service.retrieveActorRef(destination, Set(waiter.ref))
179+
180+
// Nothing was asked and nobody was told: the lookup simply did not happen.
181+
parent.expectNoMessage(100.millis)
182+
waiter.expectNoMessage(100.millis)
183+
184+
// ...and the id was not recorded as queried, so the next message bound for it re-asks. Marking
185+
// it would strand every message for that destination: the reply that clears the stash only ever
186+
// arrives in response to a `GetActorRef` that was actually sent.
187+
service.retrieveActorRef(destination, Set(waiter.ref))
188+
189+
assert(parent.expectMsgType[GetActorRef].id == destination)
190+
}
191+
192+
it should "contain a parent lookup that keeps failing, not just its first failure" in {
193+
val parent = TestProbe()
194+
val destination = ActorVirtualIdentity("persistently-unreachable-parent-destination")
195+
val waiter = TestProbe()
196+
val actorService = new FailingParentActorService(workerId, newContext(parent))
197+
val service = new PekkoActorRefMappingService(actorService)
198+
// Every read of `parent` throws, which is the realistic shape of an unreachable parent: the
199+
// condition is a property of the actor, not of one attempt. A handler that reads the failing
200+
// value a second time (e.g. to name the parent ref in its own log line) therefore throws out
201+
// of the very try/catch that exists to contain the failure.
202+
actorService.failuresLeft = Int.MaxValue
203+
204+
service.retrieveActorRef(destination, Set(waiter.ref))
205+
206+
parent.expectNoMessage(100.millis)
207+
waiter.expectNoMessage(100.millis)
208+
209+
// Once the parent recovers, the id must still be askable -- the contained failure may not have
210+
// marked it queried.
211+
actorService.failuresLeft = 0
212+
service.retrieveActorRef(destination, Set(waiter.ref))
213+
214+
assert(parent.expectMsgType[GetActorRef].id == destination)
215+
}
216+
}
217+
218+
/** Minimal actor used only to obtain a live [[ActorContext]] from Pekko TestKit. */
219+
class ActorRefMappingServiceContextHolder extends Actor {
220+
override def receive: Receive = { case _ => () }
221+
}
222+
223+
/** A [[PekkoActorService]] whose first `failuresLeft` parent lookups throw. */
224+
class FailingParentActorService(id: ActorVirtualIdentity, actorContext: ActorContext)
225+
extends PekkoActorService(id, actorContext) {
226+
227+
var failuresLeft: Int = 0
228+
229+
override def parent: ActorRef = {
230+
if (failuresLeft > 0) {
231+
failuresLeft -= 1
232+
throw new IllegalStateException("parent is unreachable")
233+
}
234+
super.parent
235+
}
236+
}

0 commit comments

Comments
 (0)