Xiao-zhen-Liu commented on code in PR #5006:
URL: https://github.com/apache/texera/pull/5006#discussion_r3221930143
##########
common/workflow-core/src/test/scala/org/apache/texera/amber/util/VirtualIdentityUtilsSpec.scala:
##########
@@ -99,16 +99,13 @@ class VirtualIdentityUtilsSpec extends AnyFlatSpec with
Matchers {
VirtualIdentityUtils.getWorkerIndex(actor) shouldBe 42
}
- it should "throw MatchError on non-worker actor names (current behavior)" in
{
- // getWorkerIndex pattern-matches on workerNamePattern with no fallback,
- // so passing a special ActorVirtualIdentity like CONTROLLER or SELF
- // yields scala.MatchError. Pinning this behavior here means a future
- // change that adds a fallback (or a different exception) breaks this
- // spec on purpose so the new contract is reviewed.
+ it should "fall back to -1 for non-worker actor names" in {
+ // Special ActorVirtualIdentity values like CONTROLLER or SELF do not
+ // match workerNamePattern. getWorkerIndex returns -1 as a sentinel
+ // rather than throwing scala.MatchError, mirroring the graceful
+ // handling in getPhysicalOpId and toShorterString.
val controller = ActorVirtualIdentity("CONTROLLER")
- assertThrows[scala.MatchError] {
- VirtualIdentityUtils.getWorkerIndex(controller)
- }
+ VirtualIdentityUtils.getWorkerIndex(controller) shouldBe -1
Review Comment:
nit: comment mentions "CONTROLLER or SELF", but only `CONTROLLER` is
asserted. One extra line to match:
```scala
VirtualIdentityUtils.getWorkerIndex(ActorVirtualIdentity("SELF")) shouldBe -1
```
##########
common/workflow-core/src/main/scala/org/apache/texera/amber/util/VirtualIdentityUtils.scala:
##########
@@ -72,6 +72,9 @@ object VirtualIdentityUtils {
workerId.name match {
case workerNamePattern(_, _, _, idx) =>
idx.toInt
+ case _ =>
+ // for special actorId such as SELF, CONTROLLER
+ -1
Review Comment:
Could we either document the `-1` sentinel in a Scaladoc, or return
`Option[Int]` instead?
Two existing callers make this awkward:
- `ExecutorDeployment.scala:65` does `allAddresses(workerIndex %
allAddresses.length)`. `-1 % n == -1` in Scala, so a non-worker id would turn a
clear `MatchError` into an `IndexOutOfBoundsException` one frame deeper.
- `OutputManager.scala:292` uses `getWorkerIndex(actorId).toString` as a
per-writer name — every non-worker actor would silently get the name `"-1"`.
Neither fires today (both paths only see real workers), but the new contract
makes silent misuse easier.
Options:
- **`Option[Int]`** — ~5 call sites, all in `amber/`. Forces each to
acknowledge the non-worker case.
- **Scaladoc** — keep `-1`, but note it's a sentinel and must not be used in
modulo/indexing unguarded.
Either is fine.
--
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]
