Yicong-Huang commented on code in PR #7122:
URL: https://github.com/apache/texera/pull/7122#discussion_r3773319242
##########
amber/src/test/scala/org/apache/texera/amber/engine/architecture/scheduling/RegionExecutionManagerTestSupport.scala:
##########
@@ -164,6 +165,18 @@ object RegionExecutionManagerTestSupport {
override def initState(): Unit = ()
override def loadFromCheckpoint(chkpt: CheckpointState): Unit = ()
+
+ override def receive: Receive = {
+ case Coordinator.CleanupWorkerChannels(workerIds) =>
Review Comment:
Verified: the three removals now live only in
`Coordinator.cleanupWorkerChannels` (`Coordinator.scala:45-55`), and the
harness calls that helper (`RegionExecutionManagerTestSupport.scala:171-179`),
so the stale-channel assertion exercises production logic again. The `case msg
=> super.receive(msg)` catch-all is gone too. Resolving.
The harness still carries its own copy of the `receive` wiring, so deleting
the case from `Coordinator.receive` leaves the suite green — a smaller gap than
the original one, raised separately in this round.
##########
amber/src/test/scala/org/apache/texera/amber/engine/architecture/scheduling/RegionExecutionManagerSpec.scala:
##########
@@ -234,10 +254,11 @@ class RegionExecutionManagerSpec
assert(fixture.rpcProbe.endWorkerCalls.size == fixture.workerIds.size * 2)
}
- it should "default to a bounded ~1.4s termination budget" in {
- // 4 attempts from a 200 ms base, doubling: 200 + 400 + 800 ms = ~1.4 s of
waiting, not the
- // former 150 x 200 ms (~30 s). This is the documented contract for how
long a stuck region
- // blocks before failing loudly; pin it so changes are deliberate.
+ it should "default to a bounded ~25.4s termination budget" in {
+ // 4 attempts from a 200 ms base, doubling: 200 + 400 + 800 ms = ~1.4 s of
backoff, plus
+ // a 6 s timeout per attempt. Worst-case teardown is now ~25.4 s. This is
the documented
+ // contract for how long a stuck region blocks before failing loudly; pin
it so changes
+ // are deliberate.
assert(RegionExecutionManager.DefaultMaxTerminationAttempts == 4)
assert(RegionExecutionManager.DefaultKillRetryBaseBackoffMs == 200L)
Review Comment:
Half of this landed: `DefaultTerminationTimeoutMs` is now asserted (`:284`).
The figure is still wrong, in the other direction now.
The suggestion I gave was written against the two-`.within` shape; this
revision collapsed them into one at `RegionExecutionManager.scala:225`, so an
attempt bounds one 6 s window with gracefulStop nested inside it, not two
sequential stages. `~11 s per attempt, ~45 s overall` (`:277-281`) now
contradicts `~25.4s` in the class comment
(`RegionExecutionManager.scala:74-76`). Leaving open until the two agree.
--
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]
